Implements a mechanism to lock/unlock the SortedListProvider and bags-list iter

substrate/frame/bags-list/src/lib.rs

@@ -81,6 +81,9 @@
 //! - if an item's score changes to a value no longer within the range of its current bag the item's
 //!   position will need to be updated by an external actor with rebag (update), or removal and
 //!   insertion.
+//! - the bags list supports setting/unsetting a lock on list order changes. If [`LockOrder`] is
+//!   set, all the [`SortedListProvider`] and extrinsic calls that *may* change the ordering of the
+//!   list, will fail with [`ListError::ReorderingNotAllowed`].
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
@@ -263,6 +266,12 @@ pub mod pallet {
 	pub(crate) type ListBags<T: Config<I>, I: 'static = ()> =
 		StorageMap<_, Twox64Concat, T::Score, list::Bag<T, I>>;
 
+	/// State of the lock for implicit and explicit ordering of the IDs in the bags list.
+	///
+	/// When the lock is set, no item re-ordering should happen.
+	#[pallet::storage]
+	pub(crate) type LockOrder<T: Config<I>, I: 'static = ()> = StorageValue<_, (), OptionQuery>;
+
 	#[pallet::event]
 	#[pallet::generate_deposit(pub(crate) fn deposit_event)]
 	pub enum Event<T: Config<I>, I: 'static = ()> {
@@ -277,6 +286,8 @@ pub mod pallet {
 	pub enum Error<T, I = ()> {
 		/// A error in the list interface implementation.
 		List(ListError),
+		/// Explicit re-ordering is currently locked.
+		ReorderingLocked,
 	}
 
 	impl<T, I> From<ListError> for Error<T, I> {
@@ -301,6 +312,9 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::rebag_non_terminal().max(T::WeightInfo::rebag_terminal()))]
 		pub fn rebag(origin: OriginFor<T>, dislocated: AccountIdLookupOf<T>) -> DispatchResult {
 			ensure_signed(origin)?;
+
+			ensure!(LockOrder::<T, I>::get().is_none(), Error::<T, I>::ReorderingLocked);
+
 			let dislocated = T::Lookup::lookup(dislocated)?;
 			let current_score = T::ScoreProvider::score(&dislocated);
 			let _ = Pallet::<T, I>::do_rebag(&dislocated, current_score)
@@ -324,6 +338,8 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			lighter: AccountIdLookupOf<T>,
 		) -> DispatchResult {
+			ensure!(LockOrder::<T, I>::get().is_none(), Error::<T, I>::ReorderingLocked);
+
 			let heavier = ensure_signed(origin)?;
 			let lighter = T::Lookup::lookup(lighter)?;
 			List::<T, I>::put_in_front_of(&lighter, &heavier)
@@ -341,6 +357,8 @@ pub mod pallet {
 			heavier: AccountIdLookupOf<T>,
 			lighter: AccountIdLookupOf<T>,
 		) -> DispatchResult {
+			ensure!(LockOrder::<T, I>::get().is_none(), Error::<T, I>::ReorderingLocked);
+
 			let _ = ensure_signed(origin)?;
 			let lighter = T::Lookup::lookup(lighter)?;
 			let heavier = T::Lookup::lookup(heavier)?;
@@ -422,20 +440,54 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		List::<T, I>::contains(id)
 	}
 
+	// TODO: from another perspective, should we prevent node inserts/removes to happen when the
+	// lock is set? this is the easiest way, but staking needs to either be aware of it and buffer
+	// requests or it is a bad UX for stakers. or the buffer can happen at the bags list level.
+	// another option is to be more fancier way is to  freeze the iterator when the lock is set.
+
 	fn on_insert(id: T::AccountId, score: T::Score) -> Result<(), ListError> {
-		List::<T, I>::insert(id, score)
+		match LockOrder::<T, I>::get() {
+			Some(_) => Err(ListError::ReorderingNotAllowed),
+			None => List::<T, I>::insert(id, score),
+		}
 	}
 
 	fn get_score(id: &T::AccountId) -> Result<T::Score, ListError> {
 		List::<T, I>::get_score(id)
 	}
 
 	fn on_update(id: &T::AccountId, new_score: T::Score) -> Result<(), ListError> {
-		Pallet::<T, I>::do_rebag(id, new_score).map(|_| ())
+		match LockOrder::<T, I>::get() {
+			Some(_) => Err(ListError::ReorderingNotAllowed),
+			None => Pallet::<T, I>::do_rebag(id, new_score).map(|_| ()),
+		}
 	}
 
 	fn on_remove(id: &T::AccountId) -> Result<(), ListError> {
-		List::<T, I>::remove(id)
+		match LockOrder::<T, I>::get() {
+			Some(_) => Err(ListError::ReorderingNotAllowed),
+			None => List::<T, I>::remove(id),
+		}
+	}
+
+	fn lock_ordering() -> Result<(), ListError> {
+		LockOrder::<T, I>::mutate(|current| match current {
+			Some(_) => Err(ListError::LockAlreadySet),
+			None => {
+				*current = Some(());
+				Ok(())
+			},
+		})
+	}
+
+	fn unlock_ordering() -> Result<(), ListError> {
+		LockOrder::<T, I>::mutate(|current| match current {
+			Some(_) => {
+				*current = None;
+				Ok(())
+			},
+			None => Err(ListError::LockAlreadyUnset),
+		})
 	}
 
 	fn unsafe_regenerate(

substrate/frame/bags-list/src/list/mod.rs

@@ -61,6 +61,12 @@ pub enum ListError {
 	NotInSameBag,
 	/// Given node id was not found.
 	NodeNotFound,
+	/// An action that affects ordering cannot complete since re-ordering is disabled.
+	ReorderingNotAllowed,
+	/// List lock is already set.
+	LockAlreadySet,
+	/// List lock is already released.
+	LockAlreadyUnset,
 }
 
 #[cfg(test)]

substrate/frame/bags-list/src/tests.rs

@@ -524,6 +524,119 @@ mod pallet {
 			);
 		});
 	}
+
+	#[test]
+	fn lock_storage_state_and_errors_work() {
+		ExtBuilder::default().build_and_execute(|| {
+			// lock is unset.
+			assert!(LockOrder::<Runtime>::get().is_none());
+
+			assert_ok!(BagsList::lock_ordering());
+			// lock has been set.
+			assert!(LockOrder::<Runtime>::get().is_some());
+			// locking again will result in an error.
+			assert_noop!(BagsList::lock_ordering(), ListError::LockAlreadySet);
+
+			// unset lock
+			assert_ok!(BagsList::unlock_ordering());
+			// lock has been unset.
+			assert!(LockOrder::<Runtime>::get().is_none());
+			// unlocking again will result in an error.
+			assert_noop!(BagsList::unlock_ordering(), ListError::LockAlreadyUnset);
+		})
+	}
+
+	#[test]
+	fn rebag_with_lock_is_noop() {
+		ExtBuilder::default().add_ids(vec![(42, 20)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![42]), (1_000, vec![2, 3, 4])]
+			);
+
+			// set lock ordering.
+			assert_ok!(BagsList::lock_ordering());
+
+			// when increasing score to the level of non-existent bag
+			assert_eq!(List::<Runtime>::get_score(&42).unwrap(), 20);
+			StakingMock::set_score_of(&42, 2_000);
+
+			// new bag and rebag won't happen since lock is set.
+			assert_noop!(
+				BagsList::rebag(RuntimeOrigin::signed(0), 42),
+				crate::pallet::Error::<Runtime>::ReorderingLocked
+			);
+
+			// unlock sorting and try again.
+			assert_ok!(BagsList::unlock_ordering());
+			assert_ok!(BagsList::rebag(RuntimeOrigin::signed(0), 42));
+
+			// new bag is created and the id moves into it
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (2_000, vec![42])]
+			);
+		})
+	}
+
+	#[test]
+	fn put_in_front_of_with_lock_is_noop() {
+		ExtBuilder::default()
+			.skip_genesis_ids()
+			.add_ids(vec![(10, 15), (11, 16)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![10, 11])]);
+
+				// set lock.
+				assert_ok!(BagsList::lock_ordering());
+
+				assert_noop!(
+					BagsList::put_in_front_of(RuntimeOrigin::signed(11), 10),
+					crate::pallet::Error::<Runtime>::ReorderingLocked
+				);
+
+				// unset lock.
+				assert_ok!(BagsList::unlock_ordering());
+
+				assert_ok!(BagsList::put_in_front_of(RuntimeOrigin::signed(11), 10));
+
+				// then
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![11, 10])]);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_other_permissionless_with_lock_is_noop() {
+		ExtBuilder::default()
+			.skip_genesis_ids()
+			.add_ids(vec![(10, 15), (11, 16), (12, 19)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![10, 11, 12])]);
+				// 11 now has more weight than 10 and can be moved before it.
+				StakingMock::set_score_of(&11u64, 17);
+
+				// set lock ordering.
+				assert_ok!(BagsList::lock_ordering());
+
+				// put in front of fails due to reordering lock.
+				assert_noop!(
+					BagsList::put_in_front_of_other(RuntimeOrigin::signed(42), 11u64, 10),
+					crate::pallet::Error::<Runtime>::ReorderingLocked,
+				);
+
+				// unlock ordering.
+				assert_ok!(BagsList::unlock_ordering());
+
+				// now put in front of works as expected.
+				assert_ok!(BagsList::put_in_front_of_other(RuntimeOrigin::signed(42), 11u64, 10));
+
+				// then
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![11, 10, 12])]);
+			});
+	}
 }
 
 mod sorted_list_provider {
@@ -735,4 +848,29 @@ mod sorted_list_provider {
 			assert!(non_existent_ids.iter().all(|id| !BagsList::contains(id)));
 		})
 	}
+
+	#[test]
+	fn lock_works() {
+		ExtBuilder::default().build_and_execute(|| {
+			// locks list ordering changes.
+			assert_ok!(BagsList::lock_ordering());
+
+			// on_insert fails because it will change the ordering of the list.
+			assert!(!get_list_as_ids().contains(&10));
+			assert_noop!(BagsList::on_insert(10, 100), ListError::ReorderingNotAllowed);
+
+			// on_update fails because it may change the ordering of the list.
+			assert!(get_list_as_ids().contains(&1));
+			assert_noop!(BagsList::on_update(&1, 100), ListError::ReorderingNotAllowed);
+
+			// on_remove fails because it may change the ordering of the list.
+			assert_noop!(BagsList::on_remove(&1), ListError::ReorderingNotAllowed);
+
+			// unlock ordering and confirm all the above actions work now.
+			assert_ok!(BagsList::unlock_ordering());
+			assert_ok!(BagsList::on_insert(10, 100));
+			assert_ok!(BagsList::on_update(&1, 100));
+			assert_ok!(BagsList::on_remove(&1));
+		})
+	}
 }

substrate/frame/election-provider-support/src/lib.rs

@@ -548,6 +548,14 @@ pub trait SortedListProvider<AccountId> {
 	/// Returns `Ok(())` iff it successfully removes an item, an `Err(_)` otherwise.
 	fn on_remove(id: &AccountId) -> Result<(), Self::Error>;
 
+	/// Locks the explicit re-ordering of the provider list.
+	///
+	/// When locked, the list item's should not be re-ordered.
+	fn lock_ordering() -> Result<(), Self::Error>;
+
+	/// Unlocks the re-ordering of the provider list.
+	fn unlock_ordering() -> Result<(), Self::Error>;
+
 	/// Regenerate this list from scratch. Returns the count of items inserted.
 	///
 	/// This should typically only be used at a runtime upgrade.