Migrate PendingVerification in pallet_registrar from value to map (#528)

* Migrate PendingVerification in pallet_registrar from value to map

* Fix benchmarks

* typescript-api

* Add test for migration

pallets/registrar/src/benchmarks.rs

@@ -61,6 +61,11 @@ mod benchmarks {
         }
     }
 
+    // Returns number of para ids in pending verification (registered but not marked as valid)
+    fn pending_verification_len<T: Config>() -> usize {
+        crate::PendingVerification::<T>::iter_keys().count()
+    }
+
     #[benchmark]
     fn register(x: Linear<5, 3_000_000>, y: Linear<1, 50>, z: Linear<1, 10>) {
         let mut data = vec![];
@@ -84,7 +89,7 @@ mod benchmarks {
         }
 
         // We should have registered y-1
-        assert_eq!(Pallet::<T>::pending_verification().len(), (y - 1) as usize);
+        assert_eq!(pending_verification_len::<T>(), (y - 1) as usize);
 
         let (caller, _deposit_amount) =
             create_funded_user::<T>("caller", 0, T::DepositAmount::get());
@@ -93,7 +98,7 @@ mod benchmarks {
         Pallet::<T>::register(RawOrigin::Signed(caller), Default::default(), storage);
 
         // verification code
-        assert_eq!(Pallet::<T>::pending_verification().len(), y as usize);
+        assert_eq!(pending_verification_len::<T>(), y as usize);
         assert!(Pallet::<T>::registrar_deposit(ParaId::default()).is_some());
     }
 
@@ -116,14 +121,14 @@ mod benchmarks {
         }
 
         // We should have registered y
-        assert_eq!(Pallet::<T>::pending_verification().len(), y as usize);
+        assert_eq!(pending_verification_len::<T>(), y as usize);
         assert!(Pallet::<T>::registrar_deposit(ParaId::from(y - 1)).is_some());
 
         #[extrinsic_call]
         Pallet::<T>::deregister(RawOrigin::Root, (y - 1).into());
 
         // We should have y-1
-        assert_eq!(Pallet::<T>::pending_verification().len(), (y - 1) as usize);
+        assert_eq!(pending_verification_len::<T>(), (y - 1) as usize);
         assert!(Pallet::<T>::registrar_deposit(ParaId::from(y - 1)).is_none());
     }
 
@@ -217,14 +222,14 @@ mod benchmarks {
         Pallet::<T>::initializer_on_new_session(&T::SessionDelay::get());
 
         // We should have registered y
-        assert_eq!(Pallet::<T>::pending_verification().len(), y as usize);
+        assert_eq!(pending_verification_len::<T>(), y as usize);
         T::RegistrarHooks::benchmarks_ensure_valid_for_collating((y - 1).into());
 
         #[extrinsic_call]
         Pallet::<T>::mark_valid_for_collating(RawOrigin::Root, (y - 1).into());
 
         // We should have y-1
-        assert_eq!(Pallet::<T>::pending_verification().len(), (y - 1) as usize);
+        assert_eq!(pending_verification_len::<T>(), (y - 1) as usize);
     }
 
     #[benchmark]
@@ -380,7 +385,7 @@ mod benchmarks {
         }
 
         // We should have registered y-1
-        assert_eq!(Pallet::<T>::pending_verification().len(), (y - 1) as usize);
+        assert_eq!(pending_verification_len::<T>(), (y - 1) as usize);
 
         let (caller, _deposit_amount) =
             create_funded_user::<T>("caller", 0, T::DepositAmount::get());
@@ -394,7 +399,7 @@ mod benchmarks {
         );
 
         // verification code
-        assert_eq!(Pallet::<T>::pending_verification().len(), y as usize);
+        assert_eq!(pending_verification_len::<T>(), y as usize);
         assert!(Pallet::<T>::registrar_deposit(ParaId::default()).is_some());
     }
 

pallets/registrar/src/lib.rs

@@ -173,7 +173,7 @@ pub mod pallet {
     #[pallet::storage]
     #[pallet::getter(fn pending_verification)]
     pub type PendingVerification<T: Config> =
-        StorageValue<_, BoundedVec<ParaId, T::MaxLengthParaIds>, ValueQuery>;
+        StorageMap<_, Blake2_128Concat, ParaId, (), OptionQuery>;
 
     #[pallet::storage]
     #[pallet::getter(fn paused)]
@@ -279,7 +279,7 @@ pub mod pallet {
             // Get all those para ids and check for duplicates
             let mut para_ids: Vec<ParaId> = vec![];
             para_ids.extend(RegisteredParaIds::<T>::get());
-            para_ids.extend(PendingVerification::<T>::get());
+            para_ids.extend(PendingVerification::<T>::iter_keys());
             para_ids.extend(Paused::<T>::get());
             para_ids.sort();
             para_ids.dedup_by(|a, b| {
@@ -343,7 +343,6 @@ pub mod pallet {
                 );
             }
             assert_is_sorted_and_unique(&RegisteredParaIds::<T>::get(), "RegisteredParaIds");
-            assert_is_sorted_and_unique(&PendingVerification::<T>::get(), "PendingVerification");
             assert_is_sorted_and_unique(&Paused::<T>::get(), "Paused");
             for (i, (_session_index, x)) in PendingParaIds::<T>::get().into_iter().enumerate() {
                 assert_is_sorted_and_unique(&x, &format!("PendingParaIds[{}]", i));
@@ -410,10 +409,8 @@ pub mod pallet {
 
             // Check if the para id is in "PendingVerification".
             // This is a special case because then we can remove it immediately, instead of waiting 2 sessions.
-            let mut para_ids = PendingVerification::<T>::get();
-            if let Ok(index) = para_ids.binary_search(&para_id) {
-                para_ids.remove(index);
-                PendingVerification::<T>::put(para_ids);
+            let is_pending_verification = PendingVerification::<T>::take(para_id).is_some();
+            if is_pending_verification {
                 Self::deposit_event(Event::ParaIdDeregistered { para_id });
                 // Cleanup immediately
                 Self::cleanup_deregistered_para_id(para_id);
@@ -454,14 +451,10 @@ pub mod pallet {
         pub fn mark_valid_for_collating(origin: OriginFor<T>, para_id: ParaId) -> DispatchResult {
             T::RegistrarOrigin::ensure_origin(origin)?;
 
-            let mut pending_verification = PendingVerification::<T>::get();
-
-            match pending_verification.binary_search(&para_id) {
-                Ok(i) => {
-                    pending_verification.remove(i);
-                }
-                Err(_) => return Err(Error::<T>::ParaIdNotInPendingVerification.into()),
-            };
+            let is_pending_verification = PendingVerification::<T>::take(para_id).is_some();
+            if !is_pending_verification {
+                return Err(Error::<T>::ParaIdNotInPendingVerification.into());
+            }
 
             Self::schedule_parachain_change(|para_ids| {
                 // We don't want to add duplicate para ids, so we check whether the potential new
@@ -481,7 +474,6 @@ pub mod pallet {
                 Ok(())
             })?;
 
-            PendingVerification::<T>::put(pending_verification);
             T::RegistrarHooks::check_valid_for_collating(para_id)?;
 
             Self::deposit_event(Event::ParaIdValidForCollating { para_id });
@@ -625,7 +617,7 @@ pub mod pallet {
 
                 /// Create a funded user.
                 /// Used for generating the necessary amount for registering
-                fn create_funded_user<T: crate::Config>(
+                fn create_funded_user<T: Config>(
                     string: &'static str,
                     n: u32,
                     total: DepositBalanceOf<T>,
@@ -671,18 +663,15 @@ pub mod pallet {
                 return Err(Error::<T>::ParaIdAlreadyRegistered.into());
             }
 
-            // Insert para id into PendingVerification
-            let mut pending_verification = PendingVerification::<T>::get();
-            match pending_verification.binary_search(&para_id) {
-                // This Ok is unreachable
-                Ok(_) => return Err(Error::<T>::ParaIdAlreadyRegistered.into()),
-                Err(index) => {
-                    pending_verification
-                        .try_insert(index, para_id)
-                        .map_err(|_e| Error::<T>::ParaIdListFull)?;
-                }
+            // Check if the para id is already in PendingVerification (unreachable)
+            let is_pending_verification = PendingVerification::<T>::take(para_id).is_some();
+            if is_pending_verification {
+                return Err(Error::<T>::ParaIdAlreadyRegistered.into());
             }
 
+            // Insert para id into PendingVerification
+            PendingVerification::<T>::insert(para_id, ());
+
             // The actual registration takes place 2 sessions after the call to
             // `mark_valid_for_collating`, but the genesis data is inserted now.
             // This is because collators should be able to start syncing the new container chain
@@ -709,7 +698,6 @@ pub mod pallet {
                 },
             );
             ParaGenesisData::<T>::insert(para_id, genesis_data);
-            PendingVerification::<T>::put(pending_verification);
 
             Ok(())
         }

runtime/common/src/migrations.rs

@@ -38,6 +38,7 @@
 use frame_support::ensure;
 
 use {
+    cumulus_primitives_core::ParaId,
     frame_support::{
         pallet_prelude::GetStorageVersion,
         traits::{OnRuntimeUpgrade, PalletInfoAccess, StorageVersion},
@@ -377,6 +378,40 @@ where
     }
 }
 
+pub struct RegistrarPendingVerificationValueToMap<T>(pub PhantomData<T>);
+impl<T> Migration for RegistrarPendingVerificationValueToMap<T>
+where
+    T: pallet_registrar::Config,
+{
+    fn friendly_name(&self) -> &str {
+        "TM_RegistrarPendingVerificationValueToMap"
+    }
+
+    fn migrate(&self, _available_weight: Weight) -> Weight {
+        let para_ids: Vec<ParaId> = frame_support::storage::unhashed::take(
+            &frame_support::storage::storage_prefix(b"Registrar", b"PendingVerification"),
+        )
+        .unwrap_or_default();
+
+        for para_id in para_ids {
+            pallet_registrar::PendingVerification::<T>::insert(para_id, ());
+        }
+
+        Weight::default()
+    }
+
+    #[cfg(feature = "try-runtime")]
+    fn pre_upgrade(&self) -> Result<Vec<u8>, sp_runtime::DispatchError> {
+        Ok(vec![])
+    }
+
+    // Run a standard post-runtime test. This works the same way as in a normal runtime upgrade.
+    #[cfg(feature = "try-runtime")]
+    fn post_upgrade(&self, _state: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
+        Ok(())
+    }
+}
+
 pub struct FlashboxMigrations<Runtime>(PhantomData<Runtime>);
 
 impl<Runtime> GetMigrations for FlashboxMigrations<Runtime>
@@ -396,6 +431,8 @@ where
 
         let migrate_add_collator_assignment_credits =
             MigrateServicesPaymentAddCollatorAssignmentCredits::<Runtime>(Default::default());
+        let migrate_registrar_pending_verification =
+            RegistrarPendingVerificationValueToMap::<Runtime>(Default::default());
 
         vec![
             // Applied in runtime 400
@@ -405,6 +442,7 @@ where
             // Applied in runtime 400
             Box::new(migrate_config_parathread_params),
             Box::new(migrate_add_collator_assignment_credits),
+            Box::new(migrate_registrar_pending_verification),
         ]
     }
 }
@@ -439,6 +477,8 @@ where
         let migrate_add_collator_assignment_credits =
             MigrateServicesPaymentAddCollatorAssignmentCredits::<Runtime>(Default::default());
         let migrate_xcmp_queue_v4 = XcmpQueueMigrationV4::<Runtime>(Default::default());
+        let migrate_registrar_pending_verification =
+            RegistrarPendingVerificationValueToMap::<Runtime>(Default::default());
         vec![
             // Applied in runtime 200
             //Box::new(migrate_invulnerables),
@@ -459,6 +499,7 @@ where
             Box::new(migrate_config_parathread_params),
             Box::new(migrate_add_collator_assignment_credits),
             Box::new(migrate_xcmp_queue_v4),
+            Box::new(migrate_registrar_pending_verification),
         ]
     }
 }

runtime/dancebox/tests/integration_test.rs

@@ -43,6 +43,7 @@ use {
     parity_scale_codec::Encode,
     runtime_common::migrations::{
         MigrateConfigurationParathreads, MigrateServicesPaymentAddCollatorAssignmentCredits,
+        RegistrarPendingVerificationValueToMap,
     },
     sp_consensus_aura::AURA_ENGINE_ID,
     sp_core::Get,
@@ -4068,6 +4069,35 @@ fn test_migration_config_full_rotation_period() {
         });
 }
 
+#[test]
+fn test_migration_registrar_pending_verification() {
+    ExtBuilder::default().build().execute_with(|| {
+        const REGISTRAR_PENDING_VERIFICATION_KEY: &[u8] =
+            &hex_literal::hex!("3fba98689ebed1138735e0e7a5a790ab57a35de516113188134ad8e43c6d55ec");
+
+        // Modify active config
+        let para_ids: Vec<ParaId> = vec![2000.into(), 2001.into(), 2002.into(), 3000.into()];
+        frame_support::storage::unhashed::put(REGISTRAR_PENDING_VERIFICATION_KEY, &para_ids);
+
+        let migration = RegistrarPendingVerificationValueToMap::<Runtime>(Default::default());
+        migration.migrate(Default::default());
+
+        let empty_key =
+            frame_support::storage::unhashed::get_raw(REGISTRAR_PENDING_VERIFICATION_KEY);
+        assert_eq!(empty_key, None);
+
+        for para_id in para_ids {
+            let exists_in_map =
+                pallet_registrar::PendingVerification::<Runtime>::get(para_id).is_some();
+            assert!(
+                exists_in_map,
+                "After migration, para id {:?} does not exist in storage map",
+                para_id
+            );
+        }
+    });
+}
+
 #[test]
 fn test_collator_assignment_gives_priority_to_invulnerables() {
     // Set max_collators = 2, take 1 invulnerable and the rest from staking