Removes ReportsByKindIndex (paritytech#13936)

* Removes ReportsByKind

* Minor build fixes

* adds migration

* Addresses review comment

* Uses clear but weight check fails

* Uses unique

* Updates test to commit the change before migration

* Uses reads_writes

* ".git/.scripts/commands/fmt/fmt.sh"

* Fixes build

* Addresses review comments

* ".git/.scripts/commands/fmt/fmt.sh"

* fixes typo

---------

Co-authored-by: command-bot <>

frame/offences/src/lib.rs

@@ -26,7 +26,9 @@ pub mod migration;
 mod mock;
 mod tests;
 
-use codec::{Decode, Encode};
+use core::marker::PhantomData;
+
+use codec::Encode;
 use frame_support::weights::Weight;
 use sp_runtime::{traits::Hash, Perbill};
 use sp_staking::{
@@ -43,12 +45,17 @@ type OpaqueTimeSlot = Vec<u8>;
 /// A type alias for a report identifier.
 type ReportIdOf<T> = <T as frame_system::Config>::Hash;
 
+const LOG_TARGET: &str = "runtime::offences";
+
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
 	use frame_support::pallet_prelude::*;
 
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
@@ -85,21 +92,6 @@ pub mod pallet {
 		ValueQuery,
 	>;
 
-	/// Enumerates all reports of a kind along with the time they happened.
-	///
-	/// All reports are sorted by the time of offence.
-	///
-	/// Note that the actual type of this mapping is `Vec<u8>`, this is because values of
-	/// different types are not supported at the moment so we are doing the manual serialization.
-	#[pallet::storage]
-	pub type ReportsByKindIndex<T> = StorageMap<
-		_,
-		Twox64Concat,
-		Kind,
-		Vec<u8>, // (O::TimeSlot, ReportIdOf<T>)
-		ValueQuery,
-	>;
-
 	/// Events type.
 	#[pallet::event]
 	#[pallet::generate_deposit(pub(super) fn deposit_event)]
@@ -190,7 +182,7 @@ impl<T: Config> Pallet<T> {
 					OffenceDetails { offender, reporters: reporters.clone() },
 				);
 
-				storage.insert(time_slot, report_id);
+				storage.insert(report_id);
 			}
 		}
 
@@ -225,38 +217,27 @@ struct TriageOutcome<T: Config> {
 struct ReportIndexStorage<T: Config, O: Offence<T::IdentificationTuple>> {
 	opaque_time_slot: OpaqueTimeSlot,
 	concurrent_reports: Vec<ReportIdOf<T>>,
-	same_kind_reports: Vec<(O::TimeSlot, ReportIdOf<T>)>,
+	_phantom: PhantomData<O>,
 }
 
 impl<T: Config, O: Offence<T::IdentificationTuple>> ReportIndexStorage<T, O> {
 	/// Preload indexes from the storage for the specific `time_slot` and the kind of the offence.
 	fn load(time_slot: &O::TimeSlot) -> Self {
 		let opaque_time_slot = time_slot.encode();
 
-		let same_kind_reports = ReportsByKindIndex::<T>::get(&O::ID);
-		let same_kind_reports =
-			Vec::<(O::TimeSlot, ReportIdOf<T>)>::decode(&mut &same_kind_reports[..])
-				.unwrap_or_default();
-
 		let concurrent_reports = <ConcurrentReportsIndex<T>>::get(&O::ID, &opaque_time_slot);
 
-		Self { opaque_time_slot, concurrent_reports, same_kind_reports }
+		Self { opaque_time_slot, concurrent_reports, _phantom: Default::default() }
 	}
 
 	/// Insert a new report to the index.
-	fn insert(&mut self, time_slot: &O::TimeSlot, report_id: ReportIdOf<T>) {
-		// Insert the report id into the list while maintaining the ordering by the time
-		// slot.
-		let pos = self.same_kind_reports.partition_point(|(when, _)| when <= time_slot);
-		self.same_kind_reports.insert(pos, (time_slot.clone(), report_id));
-
+	fn insert(&mut self, report_id: ReportIdOf<T>) {
 		// Update the list of concurrent reports.
 		self.concurrent_reports.push(report_id);
 	}
 
 	/// Dump the indexes to the storage.
 	fn save(self) {
-		ReportsByKindIndex::<T>::insert(&O::ID, self.same_kind_reports.encode());
 		<ConcurrentReportsIndex<T>>::insert(
 			&O::ID,
 			&self.opaque_time_slot,

frame/offences/src/migration.rs

@@ -15,11 +15,84 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use super::{Config, OffenceDetails, Perbill, SessionIndex};
-use frame_support::{pallet_prelude::ValueQuery, storage_alias, traits::Get, weights::Weight};
+use super::{Config, Kind, OffenceDetails, Pallet, Perbill, SessionIndex, LOG_TARGET};
+use frame_support::{
+	dispatch::GetStorageVersion,
+	pallet_prelude::ValueQuery,
+	storage_alias,
+	traits::{Get, OnRuntimeUpgrade},
+	weights::Weight,
+	Twox64Concat,
+};
 use sp_staking::offence::{DisableStrategy, OnOffenceHandler};
 use sp_std::vec::Vec;
 
+#[cfg(feature = "try-runtime")]
+use frame_support::ensure;
+
+mod v0 {
+	use super::*;
+
+	#[storage_alias]
+	pub type ReportsByKindIndex<T: Config> = StorageMap<
+		Pallet<T>,
+		Twox64Concat,
+		Kind,
+		Vec<u8>, // (O::TimeSlot, ReportIdOf<T>)
+		ValueQuery,
+	>;
+}
+
+pub mod v1 {
+	use frame_support::traits::StorageVersion;
+
+	use super::*;
+
+	pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, &'static str> {
+			let onchain = Pallet::<T>::on_chain_storage_version();
+			ensure!(onchain < 1, "pallet_offences::MigrateToV1 migration can be deleted");
+
+			log::info!(
+				target: LOG_TARGET,
+				"Number of reports to refund and delete: {}",
+				v0::ReportsByKindIndex::<T>::iter_keys().count()
+			);
+
+			Ok(Vec::new())
+		}
+
+		fn on_runtime_upgrade() -> Weight {
+			let onchain = Pallet::<T>::on_chain_storage_version();
+
+			if onchain > 0 {
+				log::info!(target: LOG_TARGET, "pallet_offences::MigrateToV1 should be removed");
+				return T::DbWeight::get().reads(1)
+			}
+
+			let keys_removed = v0::ReportsByKindIndex::<T>::clear(u32::MAX, None).unique as u64;
+			let weight = T::DbWeight::get().reads_writes(keys_removed, keys_removed);
+
+			StorageVersion::new(1).put::<Pallet<T>>();
+
+			weight
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_state: Vec<u8>) -> Result<(), &'static str> {
+			let onchain = Pallet::<T>::on_chain_storage_version();
+			ensure!(onchain == 1, "pallet_offences::MigrateToV1 needs to be run");
+			ensure!(
+				v0::ReportsByKindIndex::<T>::iter_keys().count() == 0,
+				"there are some dangling reports that need to be destroyed and refunded"
+			);
+			Ok(())
+		}
+	}
+}
+
 /// Type of data stored as a deferred offence
 type DeferredOffenceOf<T> = (
 	Vec<OffenceDetails<<T as frame_system::Config>::AccountId, <T as Config>::IdentificationTuple>>,
@@ -36,7 +109,7 @@ type DeferredOffences<T: Config> =
 pub fn remove_deferred_storage<T: Config>() -> Weight {
 	let mut weight = T::DbWeight::get().reads_writes(1, 1);
 	let deferred = <DeferredOffences<T>>::take();
-	log::info!(target: "runtime::offences", "have {} deferred offences, applying.", deferred.len());
+	log::info!(target: LOG_TARGET, "have {} deferred offences, applying.", deferred.len());
 	for (offences, perbill, session) in deferred.iter() {
 		let consumed = T::OnOffenceHandler::on_offence(
 			offences,
@@ -53,10 +126,32 @@ pub fn remove_deferred_storage<T: Config>() -> Weight {
 #[cfg(test)]
 mod test {
 	use super::*;
-	use crate::mock::{new_test_ext, with_on_offence_fractions, Runtime as T};
+	use crate::mock::{new_test_ext, with_on_offence_fractions, Runtime as T, KIND};
+	use codec::Encode;
 	use sp_runtime::Perbill;
 	use sp_staking::offence::OffenceDetails;
 
+	#[test]
+	fn migration_to_v1_works() {
+		let mut ext = new_test_ext();
+
+		ext.execute_with(|| {
+			<v0::ReportsByKindIndex<T>>::insert(KIND, 2u32.encode());
+			assert!(<v0::ReportsByKindIndex<T>>::iter_values().count() > 0);
+		});
+
+		ext.commit_all().unwrap();
+
+		ext.execute_with(|| {
+			assert_eq!(
+				v1::MigrateToV1::<T>::on_runtime_upgrade(),
+				<T as frame_system::Config>::DbWeight::get().reads_writes(1, 1),
+			);
+
+			assert!(<v0::ReportsByKindIndex<T>>::iter_values().count() == 0);
+		})
+	}
+
 	#[test]
 	fn should_resubmit_deferred_offences() {
 		new_test_ext().execute_with(|| {

frame/offences/src/mock.rs

@@ -164,8 +164,3 @@ impl offence::Offence<u64> for Offence {
 		Perbill::from_percent(5 + offenders_count * 100 / self.validator_set_count)
 	}
 }
-
-/// Create the report id for the given `offender` and `time_slot` combination.
-pub fn report_id(time_slot: u128, offender: u64) -> H256 {
-	Offences::report_id::<Offence>(&time_slot, &offender)
-}

frame/offences/src/tests.rs

@@ -21,8 +21,8 @@
 
 use super::*;
 use crate::mock::{
-	new_test_ext, offence_reports, report_id, with_on_offence_fractions, Offence, Offences,
-	RuntimeEvent, System, KIND,
+	new_test_ext, offence_reports, with_on_offence_fractions, Offence, Offences, RuntimeEvent,
+	System, KIND,
 };
 use frame_system::{EventRecord, Phase};
 use sp_runtime::Perbill;
@@ -245,48 +245,3 @@ fn should_properly_count_offences() {
 		);
 	});
 }
-
-/// We insert offences in sorted order using the time slot in the `same_kind_reports`.
-/// This test ensures that it works as expected.
-#[test]
-fn should_properly_sort_offences() {
-	new_test_ext().execute_with(|| {
-		// given
-		let time_slot = 42;
-		assert_eq!(offence_reports(KIND, time_slot), vec![]);
-
-		let offence1 = Offence { validator_set_count: 5, time_slot, offenders: vec![5] };
-		let offence2 = Offence { validator_set_count: 5, time_slot, offenders: vec![4] };
-		let offence3 =
-			Offence { validator_set_count: 5, time_slot: time_slot + 1, offenders: vec![6, 7] };
-		let offence4 =
-			Offence { validator_set_count: 5, time_slot: time_slot - 1, offenders: vec![3] };
-		Offences::report_offence(vec![], offence1).unwrap();
-		with_on_offence_fractions(|f| {
-			assert_eq!(f.clone(), vec![Perbill::from_percent(25)]);
-			f.clear();
-		});
-
-		// when
-		// report for the second time
-		Offences::report_offence(vec![], offence2).unwrap();
-		Offences::report_offence(vec![], offence3).unwrap();
-		Offences::report_offence(vec![], offence4).unwrap();
-
-		// then
-		let same_kind_reports = Vec::<(u128, sp_core::H256)>::decode(
-			&mut &crate::ReportsByKindIndex::<crate::mock::Runtime>::get(KIND)[..],
-		)
-		.unwrap();
-		assert_eq!(
-			same_kind_reports,
-			vec![
-				(time_slot - 1, report_id(time_slot - 1, 3)),
-				(time_slot, report_id(time_slot, 5)),
-				(time_slot, report_id(time_slot, 4)),
-				(time_slot + 1, report_id(time_slot + 1, 6)),
-				(time_slot + 1, report_id(time_slot + 1, 7)),
-			]
-		);
-	});
-}