Add Score to Bags List (#11357)

* Add Score to Bags List

* fix ordering

* make compile

* in progress migration

* make migration compile

* remove old check

* remove runtime specific migration

* fix warning

* Apply suggestions from code review

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* improve migration

* fix

* fix merge

* fmt

* Update migrations.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/frame/bags-list/fuzzer/src/main.rs

@@ -70,13 +70,20 @@ fn main() {
 				},
 				Action::Update => {
 					let already_contains = BagsList::contains(&id);
-					BagsList::on_update(&id, vote_weight).unwrap();
 					if already_contains {
+						BagsList::on_update(&id, vote_weight).unwrap();
 						assert!(BagsList::contains(&id));
+					} else {
+						BagsList::on_update(&id, vote_weight).unwrap_err();
 					}
 				},
 				Action::Remove => {
-					BagsList::on_remove(&id).unwrap();
+					let already_contains = BagsList::contains(&id);
+					if already_contains {
+						BagsList::on_remove(&id).unwrap();
+					} else {
+						BagsList::on_remove(&id).unwrap_err();
+					}
 					assert!(!BagsList::contains(&id));
 				},
 			}

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

@@ -82,7 +82,10 @@ macro_rules! log {
 	($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
 		log::$level!(
 			target: crate::LOG_TARGET,
-			concat!("[{:?}] 👜", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
+			concat!("[{:?}] 👜 [{}]", $patter),
+			<frame_system::Pallet<T>>::block_number(),
+			<crate::Pallet::<T, I> as frame_support::traits::PalletInfoAccess>::name()
+			$(, $values)*
 		)
 	};
 }
@@ -189,6 +192,8 @@ pub mod pallet {
 	pub enum Event<T: Config<I>, I: 'static = ()> {
 		/// Moved an account from one bag to another.
 		Rebagged { who: T::AccountId, from: T::Score, to: T::Score },
+		/// Updated the score of some account to the given amount.
+		ScoreUpdated { who: T::AccountId, new_score: T::Score },
 	}
 
 	#[pallet::error]
@@ -212,13 +217,16 @@ pub mod pallet {
 		///
 		/// Anyone can call this function about any potentially dislocated account.
 		///
-		/// Will never return an error; if `dislocated` does not exist or doesn't need a rebag, then
-		/// it is a noop and fees are still collected from `origin`.
+		/// Will always update the stored score of `dislocated` to the correct score, based on
+		/// `ScoreProvider`.
+		///
+		/// If `dislocated` does not exists, it returns an error.
 		#[pallet::weight(T::WeightInfo::rebag_non_terminal().max(T::WeightInfo::rebag_terminal()))]
 		pub fn rebag(origin: OriginFor<T>, dislocated: T::AccountId) -> DispatchResult {
 			ensure_signed(origin)?;
 			let current_score = T::ScoreProvider::score(&dislocated);
-			let _ = Pallet::<T, I>::do_rebag(&dislocated, current_score);
+			let _ = Pallet::<T, I>::do_rebag(&dislocated, current_score)
+				.map_err::<Error<T, I>, _>(Into::into)?;
 			Ok(())
 		}
 
@@ -265,6 +273,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		if let Some((from, to)) = maybe_movement {
 			Self::deposit_event(Event::<T, I>::Rebagged { who: account.clone(), from, to });
 		};
+		Self::deposit_event(Event::<T, I>::ScoreUpdated { who: account.clone(), new_score });
 		Ok(maybe_movement)
 	}
 
@@ -302,6 +311,10 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		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(|_| ())
 	}
@@ -359,3 +372,22 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		}
 	}
 }
+
+impl<T: Config<I>, I: 'static> ScoreProvider<T::AccountId> for Pallet<T, I> {
+	type Score = <Pallet<T, I> as SortedListProvider<T::AccountId>>::Score;
+
+	fn score(id: &T::AccountId) -> T::Score {
+		Node::<T, I>::get(id).map(|node| node.score()).unwrap_or_default()
+	}
+
+	#[cfg(any(feature = "runtime-benchmarks", test))]
+	fn set_score_of(id: &T::AccountId, new_score: T::Score) {
+		ListNodes::<T, I>::mutate(id, |maybe_node| {
+			if let Some(node) = maybe_node.as_mut() {
+				node.set_score(new_score)
+			} else {
+				panic!("trying to mutate {:?} which does not exists", id);
+			}
+		})
+	}
+}

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

@@ -28,6 +28,7 @@ use crate::Config;
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_election_provider_support::ScoreProvider;
 use frame_support::{
+	ensure,
 	traits::{Defensive, Get},
 	DefaultNoBound, PalletError,
 };
@@ -38,7 +39,7 @@ use sp_std::{
 	collections::{btree_map::BTreeMap, btree_set::BTreeSet},
 	iter,
 	marker::PhantomData,
-	vec::Vec,
+	prelude::*,
 };
 
 #[derive(Debug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, PalletError)]
@@ -227,6 +228,11 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		crate::ListNodes::<T, I>::contains_key(id)
 	}
 
+	/// Get the score of the given node,
+	pub fn get_score(id: &T::AccountId) -> Result<T::Score, ListError> {
+		Node::<T, I>::get(id).map(|node| node.score()).ok_or(ListError::NodeNotFound)
+	}
+
 	/// Iterate over all nodes in all bags in the list.
 	///
 	/// Full iteration can be expensive; it's recommended to limit the number of items with
@@ -310,7 +316,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		let bag_score = notional_bag_for::<T, I>(score);
 		let mut bag = Bag::<T, I>::get_or_make(bag_score);
 		// unchecked insertion is okay; we just got the correct `notional_bag_for`.
-		bag.insert_unchecked(id.clone());
+		bag.insert_unchecked(id.clone(), score);
 
 		// new inserts are always the tail, so we must write the bag.
 		bag.put();
@@ -381,16 +387,18 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 	/// If the node was in the correct bag, no effect. If the node was in the incorrect bag, they
 	/// are moved into the correct bag.
 	///
-	/// Returns `Some((old_idx, new_idx))` if the node moved, otherwise `None`.
+	/// Returns `Some((old_idx, new_idx))` if the node moved, otherwise `None`. In both cases, the
+	/// node's score is written to the `score` field. Thus, this is not a noop, even if `None`.
 	///
 	/// This operation is somewhat more efficient than simply calling [`self.remove`] followed by
 	/// [`self.insert`]. However, given large quantities of nodes to move, it may be more efficient
 	/// to call [`self.remove_many`] followed by [`self.insert_many`].
 	pub(crate) fn update_position_for(
-		node: Node<T, I>,
+		mut node: Node<T, I>,
 		new_score: T::Score,
 	) -> Option<(T::Score, T::Score)> {
-		node.is_misplaced(new_score).then(move || {
+		node.score = new_score;
+		if node.is_misplaced(new_score) {
 			let old_bag_upper = node.bag_upper;
 
 			if !node.is_terminal() {
@@ -402,12 +410,9 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 				bag.remove_node_unchecked(&node);
 				bag.put();
 			} else {
-				crate::log!(
-					error,
-					"Node {:?} did not have a bag; ListBags is in an inconsistent state",
-					node.id,
+				frame_support::defensive!(
+					"Node did not have a bag; BagsList is in an inconsistent state"
 				);
-				debug_assert!(false, "every node must have an extant bag associated with it");
 			}
 
 			// put the node into the appropriate new bag.
@@ -418,8 +423,12 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 			bag.insert_node_unchecked(node);
 			bag.put();
 
-			(old_bag_upper, new_bag_upper)
-		})
+			Some((old_bag_upper, new_bag_upper))
+		} else {
+			// just write the new score.
+			node.put();
+			None
+		}
 	}
 
 	/// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in the
@@ -428,8 +437,6 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
 	) -> Result<(), ListError> {
-		use frame_support::ensure;
-
 		let lighter_node = Node::<T, I>::get(&lighter_id).ok_or(ListError::NodeNotFound)?;
 		let heavier_node = Node::<T, I>::get(&heavier_id).ok_or(ListError::NodeNotFound)?;
 
@@ -510,7 +517,6 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 	/// all bags and nodes are checked per *any* update to `List`.
 	#[cfg(feature = "std")]
 	pub(crate) fn sanity_check() -> Result<(), &'static str> {
-		use frame_support::ensure;
 		let mut seen_in_list = BTreeSet::new();
 		ensure!(
 			Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)),
@@ -523,7 +529,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		ensure!(iter_count == stored_count, "iter_count != stored_count");
 		ensure!(stored_count == nodes_count, "stored_count != nodes_count");
 
-		crate::log!(debug, "count of nodes: {}", stored_count);
+		crate::log!(trace, "count of nodes: {}", stored_count);
 
 		let active_bags = {
 			let thresholds = T::BagThresholds::get().iter().copied();
@@ -544,7 +550,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 			active_bags.clone().fold(0u32, |acc, cur| acc + cur.iter().count() as u32);
 		ensure!(nodes_count == nodes_in_bags_count, "stored_count != nodes_in_bags_count");
 
-		crate::log!(debug, "count of active bags {}", active_bags.count());
+		crate::log!(trace, "count of active bags {}", active_bags.count());
 
 		// check that all nodes are sane. We check the `ListNodes` storage item directly in case we
 		// have some "stale" nodes that are not in a bag.
@@ -667,7 +673,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 	///
 	/// Storage note: this modifies storage, but only for the nodes. You still need to call
 	/// `self.put()` after use.
-	fn insert_unchecked(&mut self, id: T::AccountId) {
+	fn insert_unchecked(&mut self, id: T::AccountId, score: T::Score) {
 		// insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long
 		// as this bag is the correct one, we're good. All calls to this must come after getting the
 		// correct [`notional_bag_for`].
@@ -676,6 +682,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 			prev: None,
 			next: None,
 			bag_upper: Zero::zero(),
+			score,
 			_phantom: PhantomData,
 		});
 	}
@@ -784,11 +791,6 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 		Ok(())
 	}
 
-	#[cfg(not(feature = "std"))]
-	fn sanity_check(&self) -> Result<(), &'static str> {
-		Ok(())
-	}
-
 	/// Iterate over the nodes in this bag (public for tests).
 	#[cfg(feature = "std")]
 	#[allow(dead_code)]
@@ -809,12 +811,13 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 #[scale_info(skip_type_params(T, I))]
 #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))]
 pub struct Node<T: Config<I>, I: 'static = ()> {
-	id: T::AccountId,
-	prev: Option<T::AccountId>,
-	next: Option<T::AccountId>,
-	bag_upper: T::Score,
+	pub(crate) id: T::AccountId,
+	pub(crate) prev: Option<T::AccountId>,
+	pub(crate) next: Option<T::AccountId>,
+	pub(crate) bag_upper: T::Score,
+	pub(crate) score: T::Score,
 	#[codec(skip)]
-	_phantom: PhantomData<I>,
+	pub(crate) _phantom: PhantomData<I>,
 }
 
 impl<T: Config<I>, I: 'static> Node<T, I> {
@@ -877,6 +880,11 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
 		&self.id
 	}
 
+	/// Get the current vote weight of the node.
+	pub(crate) fn score(&self) -> T::Score {
+		self.score
+	}
+
 	/// Get the underlying voter (public fo tests).
 	#[cfg(feature = "std")]
 	#[allow(dead_code)]
@@ -884,6 +892,11 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
 		&self.id
 	}
 
+	#[cfg(any(feature = "runtime-benchmarks", test))]
+	pub fn set_score(&mut self, s: T::Score) {
+		self.score = s
+	}
+
 	/// The bag this nodes belongs to (public for benchmarks).
 	#[cfg(feature = "runtime-benchmarks")]
 	#[allow(dead_code)]

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

@@ -21,20 +21,20 @@ use crate::{
 	ListBags, ListNodes,
 };
 use frame_election_provider_support::{SortedListProvider, VoteWeight};
-use frame_support::{assert_noop, assert_ok, assert_storage_noop};
+use frame_support::{assert_ok, assert_storage_noop};
+
+fn node(
+	id: AccountId,
+	prev: Option<AccountId>,
+	next: Option<AccountId>,
+	bag_upper: VoteWeight,
+) -> Node<Runtime> {
+	Node::<Runtime> { id, prev, next, bag_upper, score: bag_upper, _phantom: PhantomData }
+}
 
 #[test]
 fn basic_setup_works() {
 	ExtBuilder::default().build_and_execute(|| {
-		// syntactic sugar to create a raw node
-		let node = |id, prev, next, bag_upper| Node::<Runtime> {
-			id,
-			prev,
-			next,
-			bag_upper,
-			_phantom: PhantomData,
-		};
-
 		assert_eq!(ListNodes::<Runtime>::count(), 4);
 		assert_eq!(ListNodes::<Runtime>::iter().count(), 4);
 		assert_eq!(ListBags::<Runtime>::iter().count(), 2);
@@ -83,10 +83,10 @@ fn notional_bag_for_works() {
 	let max_explicit_threshold = *<Runtime as Config>::BagThresholds::get().last().unwrap();
 	assert_eq!(max_explicit_threshold, 10_000);
 
-	// if the max explicit threshold is less than T::Value::max_value(),
+	// if the max explicit threshold is less than T::Score::max_value(),
 	assert!(VoteWeight::MAX > max_explicit_threshold);
 
-	// then anything above it will belong to the T::Value::max_value() bag.
+	// then anything above it will belong to the T::Score::max_value() bag.
 	assert_eq!(notional_bag_for::<Runtime, _>(max_explicit_threshold), max_explicit_threshold);
 	assert_eq!(notional_bag_for::<Runtime, _>(max_explicit_threshold + 1), VoteWeight::MAX);
 }
@@ -154,6 +154,8 @@ fn migrate_works() {
 }
 
 mod list {
+	use frame_support::assert_noop;
+
 	use super::*;
 
 	#[test]
@@ -175,7 +177,7 @@ mod list {
 					]
 				);
 
-				// when adding an id that has a higher weight than pre-existing ids in the bag
+				// when adding an id that has a higher score than pre-existing ids in the bag
 				assert_ok!(List::<Runtime>::insert(7, 10));
 
 				// then
@@ -246,10 +248,7 @@ mod list {
 			assert!(get_list_as_ids().contains(&3));
 
 			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::insert(3, 20).unwrap_err(),
-				ListError::Duplicate
-			));
+			assert_noop!(List::<Runtime>::insert(3, 20), ListError::Duplicate);
 		});
 	}
 
@@ -317,37 +316,36 @@ mod list {
 		ExtBuilder::default().build_and_execute(|| {
 			// given a correctly placed account 1 at bag 10.
 			let node = Node::<Runtime>::get(&1).unwrap();
+			assert_eq!(node.score, 10);
 			assert!(!node.is_misplaced(10));
 
-			// .. it is invalid with weight 20
+			// .. it is invalid with score 20
 			assert!(node.is_misplaced(20));
 
 			// move it to bag 20.
-			assert_eq!(List::<Runtime>::update_position_for(node, 20), Some((10, 20)));
+			assert_eq!(List::<Runtime>::update_position_for(node.clone(), 20), Some((10, 20)));
+			assert_eq!(Node::<Runtime>::get(&1).unwrap().score, 20);
 
 			assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![1]), (1_000, vec![2, 3, 4])]);
 
-			// get the new updated node; try and update the position with no change in weight.
+			// get the new updated node; try and update the position with no change in score.
 			let node = Node::<Runtime>::get(&1).unwrap();
 			assert_storage_noop!(assert_eq!(
 				List::<Runtime>::update_position_for(node.clone(), 20),
 				None
 			));
 
-			// then move it to bag 1_000 by giving it weight 500.
+			// then move it to bag 1_000 by giving it score 500.
 			assert_eq!(List::<Runtime>::update_position_for(node.clone(), 500), Some((20, 1_000)));
+			assert_eq!(Node::<Runtime>::get(&1).unwrap().score, 500);
 			assert_eq!(List::<Runtime>::get_bags(), vec![(1_000, vec![2, 3, 4, 1])]);
 
 			// moving within that bag again is a noop
 			let node = Node::<Runtime>::get(&1).unwrap();
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::update_position_for(node.clone(), 750),
-				None,
-			));
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::update_position_for(node, 1_000),
-				None,
-			));
+			assert_eq!(List::<Runtime>::update_position_for(node.clone(), 750), None);
+			assert_eq!(Node::<Runtime>::get(&1).unwrap().score, 750);
+			assert_eq!(List::<Runtime>::update_position_for(node.clone(), 1_000), None,);
+			assert_eq!(Node::<Runtime>::get(&1).unwrap().score, 1_000);
 		});
 	}
 
@@ -359,7 +357,7 @@ mod list {
 
 		// make sure there are no duplicates.
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
-			Bag::<Runtime>::get(10).unwrap().insert_unchecked(2);
+			Bag::<Runtime>::get(10).unwrap().insert_unchecked(2, 10);
 			assert_eq!(List::<Runtime>::sanity_check(), Err("duplicate identified"));
 		});
 
@@ -397,6 +395,7 @@ mod list {
 				prev: None,
 				next: None,
 				bag_upper: 15,
+				score: 15,
 				_phantom: PhantomData,
 			};
 			let node_11_no_bag = Node::<Runtime> {
@@ -404,6 +403,7 @@ mod list {
 				prev: None,
 				next: None,
 				bag_upper: 15,
+				score: 15,
 				_phantom: PhantomData,
 			};
 
@@ -435,6 +435,7 @@ mod list {
 				prev: Some(1),
 				next: Some(2),
 				bag_upper: 1_000,
+				score: 1_000,
 				_phantom: PhantomData,
 			};
 			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
@@ -464,6 +465,7 @@ mod list {
 				prev: Some(4),
 				next: None,
 				bag_upper: 1_000,
+				score: 1_000,
 				_phantom: PhantomData,
 			};
 			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
@@ -493,6 +495,7 @@ mod list {
 				prev: None,
 				next: Some(2),
 				bag_upper: 1_000,
+				score: 1_000,
 				_phantom: PhantomData,
 			};
 			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
@@ -522,6 +525,7 @@ mod list {
 				prev: Some(42),
 				next: Some(42),
 				bag_upper: 1_000,
+				score: 1_000,
 				_phantom: PhantomData,
 			};
 			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
@@ -586,6 +590,7 @@ mod bags {
 				prev: None,
 				next: None,
 				bag_upper,
+				score: bag_upper,
 				_phantom: PhantomData,
 			};
 
@@ -596,7 +601,14 @@ mod bags {
 
 			assert_eq!(
 				ListNodes::<Runtime>::get(&42).unwrap(),
-				Node { bag_upper: 10, prev: Some(1), next: None, id: 42, _phantom: PhantomData }
+				Node {
+					bag_upper: 10,
+					score: 5,
+					prev: Some(1),
+					next: None,
+					id: 42,
+					_phantom: PhantomData
+				}
 			);
 		});
 	}
@@ -609,6 +621,7 @@ mod bags {
 				prev: None,
 				next: None,
 				bag_upper,
+				score: bag_upper,
 				_phantom: PhantomData,
 			};
 
@@ -636,6 +649,7 @@ mod bags {
 				prev: Some(21),
 				next: Some(101),
 				bag_upper: 20,
+				score: 20,
 				_phantom: PhantomData,
 			};
 			bag_20.insert_node_unchecked(node_61);
@@ -649,6 +663,7 @@ mod bags {
 					prev: Some(62),
 					next: None,
 					bag_upper: 20,
+					score: 20,
 					_phantom: PhantomData,
 				}
 			);
@@ -665,13 +680,6 @@ mod bags {
 	// Document improper ways `insert_node` may be getting used.
 	#[test]
 	fn insert_node_bad_paths_documented() {
-		let node = |id, prev, next, bag_upper| Node::<Runtime> {
-			id,
-			prev,
-			next,
-			bag_upper,
-			_phantom: PhantomData,
-		};
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			// when inserting a node with both prev & next pointing at an account in an incorrect
 			// bag.
@@ -684,7 +692,14 @@ mod bags {
 			// and when the node is re-fetched all the info is correct
 			assert_eq!(
 				Node::<Runtime>::get(&42).unwrap(),
-				node(42, Some(4), None, bag_1000.bag_upper)
+				Node::<Runtime> {
+					id: 42,
+					prev: Some(4),
+					next: None,
+					bag_upper: bag_1000.bag_upper,
+					score: 500,
+					_phantom: PhantomData
+				}
 			);
 		});
 
@@ -720,7 +735,14 @@ mod bags {
 			// and the re-fetched node has bad pointers
 			assert_eq!(
 				Node::<Runtime>::get(&2).unwrap(),
-				node(2, Some(4), None, bag_1000.bag_upper)
+				Node::<Runtime> {
+					id: 2,
+					prev: Some(4),
+					next: None,
+					bag_upper: bag_1000.bag_upper,
+					score: 0,
+					_phantom: PhantomData
+				},
 			);
 			//         ^^^ despite being the bags head, it has a prev
 
@@ -739,14 +761,6 @@ mod bags {
 	)]
 	fn insert_node_duplicate_tail_panics_with_debug_assert() {
 		ExtBuilder::default().build_and_execute(|| {
-			let node = |id, prev, next, bag_upper| Node::<Runtime> {
-				id,
-				prev,
-				next,
-				bag_upper,
-				_phantom: PhantomData,
-			};
-
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])],);
 			let mut bag_1000 = Bag::<Runtime>::get(1_000).unwrap();
@@ -877,6 +891,7 @@ mod bags {
 				prev: None,
 				next: Some(3),
 				bag_upper: 10, // should be 1_000
+				score: 10,
 				_phantom: PhantomData,
 			};
 			let mut bag_1000 = Bag::<Runtime>::get(1_000).unwrap();

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

@@ -17,34 +17,125 @@
 
 //! The migrations of this pallet.
 
+use codec::{Decode, Encode};
+use core::marker::PhantomData;
+use frame_election_provider_support::ScoreProvider;
 use frame_support::traits::OnRuntimeUpgrade;
+use sp_runtime::traits::Zero;
+
+#[cfg(feature = "try-runtime")]
+use frame_support::ensure;
 
 /// A struct that does not migration, but only checks that the counter prefix exists and is correct.
-pub struct CheckCounterPrefix<T: crate::Config>(sp_std::marker::PhantomData<T>);
-impl<T: crate::Config> OnRuntimeUpgrade for CheckCounterPrefix<T> {
+pub struct CheckCounterPrefix<T: crate::Config<I>, I: 'static>(sp_std::marker::PhantomData<(T, I)>);
+impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for CheckCounterPrefix<T, I> {
 	fn on_runtime_upgrade() -> frame_support::weights::Weight {
-		0
+		frame_support::weights::Weight::zero()
 	}
 
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<(), &'static str> {
-		use frame_support::ensure;
 		// The old explicit storage item.
 		#[frame_support::storage_alias]
-		type CounterForListNodes<T: crate::Config> = StorageValue<crate::Pallet<T>, u32>;
+		type CounterForListNodes<T: crate::Config<I>, I: 'static> =
+			StorageValue<crate::Pallet<T, I>, u32>;
 
 		// ensure that a value exists in the counter struct.
 		ensure!(
-			crate::ListNodes::<T>::count() == CounterForListNodes::<T>::get().unwrap(),
+			crate::ListNodes::<T, I>::count() == CounterForListNodes::<T, I>::get().unwrap(),
 			"wrong list node counter"
 		);
 
 		crate::log!(
 			info,
 			"checked bags-list prefix to be correct and have {} nodes",
-			crate::ListNodes::<T>::count()
+			crate::ListNodes::<T, I>::count()
 		);
 
 		Ok(())
 	}
 }
+
+mod old {
+	use super::*;
+	use frame_support::pallet_prelude::*;
+
+	#[derive(Encode, Decode)]
+	pub struct PreScoreNode<T: crate::Config<I>, I: 'static = ()> {
+		pub id: T::AccountId,
+		pub prev: Option<T::AccountId>,
+		pub next: Option<T::AccountId>,
+		pub bag_upper: T::Score,
+		#[codec(skip)]
+		pub _phantom: PhantomData<I>,
+	}
+
+	#[frame_support::storage_alias]
+	pub type ListNodes<T: crate::Config<I>, I: 'static> = StorageMap<
+		crate::Pallet<T, I>,
+		Twox64Concat,
+		<T as frame_system::Config>::AccountId,
+		PreScoreNode<T, I>,
+	>;
+
+	#[frame_support::storage_alias]
+	pub type CounterForListNodes<T: crate::Config<I>, I: 'static> =
+		StorageValue<crate::Pallet<T, I>, u32, ValueQuery>;
+
+	#[frame_support::storage_alias]
+	pub type TempStorage<T: crate::Config<I>, I: 'static> =
+		StorageValue<crate::Pallet<T, I>, u32, ValueQuery>;
+}
+
+/// A struct that migrates all bags lists to contain a score value.
+pub struct AddScore<T: crate::Config<I>, I: 'static = ()>(sp_std::marker::PhantomData<(T, I)>);
+impl<T: crate::Config<I>, I: 'static> OnRuntimeUpgrade for AddScore<T, I> {
+	#[cfg(feature = "try-runtime")]
+	fn pre_upgrade() -> Result<(), &'static str> {
+		// The list node data should be corrupt at this point, so this is zero.
+		ensure!(crate::ListNodes::<T, I>::iter().count() == 0, "list node data is not corrupt");
+		// We can use the helper `old::ListNode` to get the existing data.
+		let iter_node_count: u32 = old::ListNodes::<T, I>::iter().count() as u32;
+		let tracked_node_count: u32 = old::CounterForListNodes::<T, I>::get();
+		crate::log!(info, "number of nodes before: {:?} {:?}", iter_node_count, tracked_node_count);
+		ensure!(iter_node_count == tracked_node_count, "Node count is wrong.");
+		old::TempStorage::<T, I>::put(iter_node_count);
+		Ok(())
+	}
+
+	fn on_runtime_upgrade() -> frame_support::weights::Weight {
+		for (_key, node) in old::ListNodes::<T, I>::iter() {
+			let score = T::ScoreProvider::score(&node.id);
+
+			let new_node = crate::Node {
+				id: node.id.clone(),
+				prev: node.prev,
+				next: node.next,
+				bag_upper: node.bag_upper,
+				score,
+				_phantom: node._phantom,
+			};
+
+			crate::ListNodes::<T, I>::insert(node.id, new_node);
+		}
+
+		return frame_support::weights::Weight::MAX
+	}
+
+	#[cfg(feature = "try-runtime")]
+	fn post_upgrade() -> Result<(), &'static str> {
+		let node_count_before = old::TempStorage::<T, I>::take();
+		// Now, the list node data is not corrupt anymore.
+		let iter_node_count_after: u32 = crate::ListNodes::<T, I>::iter().count() as u32;
+		let tracked_node_count_after: u32 = crate::ListNodes::<T, I>::count();
+		crate::log!(
+			info,
+			"number of nodes after: {:?} {:?}",
+			iter_node_count_after,
+			tracked_node_count_after,
+		);
+		ensure!(iter_node_count_after == node_count_before, "Not all nodes were migrated.");
+		ensure!(tracked_node_count_after == iter_node_count_after, "Node count is wrong.");
+		Ok(())
+	}
+}

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

@@ -35,8 +35,10 @@ mod pallet {
 			);
 
 			// 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);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 42));
+			assert_eq!(List::<Runtime>::get_score(&42).unwrap(), 2_000);
 
 			// then a new bag is created and the id moves into it
 			assert_eq!(
@@ -53,6 +55,8 @@ mod pallet {
 				List::<Runtime>::get_bags(),
 				vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (2_000, vec![42])]
 			);
+			// but the score is updated
+			assert_eq!(List::<Runtime>::get_score(&42).unwrap(), 1_001);
 
 			// when reducing score to the level of a non-existent bag
 			StakingMock::set_score_of(&42, 30);
@@ -63,6 +67,7 @@ mod pallet {
 				List::<Runtime>::get_bags(),
 				vec![(10, vec![1]), (30, vec![42]), (1_000, vec![2, 3, 4])]
 			);
+			assert_eq!(List::<Runtime>::get_score(&42).unwrap(), 30);
 
 			// when increasing score to the level of a pre-existing bag
 			StakingMock::set_score_of(&42, 500);
@@ -73,6 +78,7 @@ mod pallet {
 				List::<Runtime>::get_bags(),
 				vec![(10, vec![1]), (1_000, vec![2, 3, 4, 42])]
 			);
+			assert_eq!(List::<Runtime>::get_score(&42).unwrap(), 500);
 		});
 	}
 
@@ -145,21 +151,20 @@ mod pallet {
 	}
 
 	#[test]
-	fn wrong_rebag_is_noop() {
+	fn wrong_rebag_errs() {
 		ExtBuilder::default().build_and_execute(|| {
 			let node_3 = list::Node::<Runtime>::get(&3).unwrap();
-			// when account 3 is _not_ misplaced with weight 500
+			// when account 3 is _not_ misplaced with score 500
 			NextVoteWeight::set(500);
 			assert!(!node_3.is_misplaced(500));
 
-			// then calling rebag on account 3 with weight 500 is a noop
+			// then calling rebag on account 3 with score 500 is a noop
 			assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 3), Ok(())));
 
 			// when account 42 is not in the list
 			assert!(!BagsList::contains(&42));
-
-			// then rebag-ing account 42 is a noop
-			assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 42), Ok(())));
+			// then rebag-ing account 42 is an error
+			assert_storage_noop!(assert!(matches!(BagsList::rebag(Origin::signed(0), 42), Err(_))));
 		});
 	}
 
@@ -182,7 +187,6 @@ mod pallet {
 	#[test]
 	fn empty_threshold_works() {
 		BagThresholds::set(Default::default()); // which is the same as passing `()` to `Get<_>`.
-
 		ExtBuilder::default().build_and_execute(|| {
 			// everyone in the same bag.
 			assert_eq!(List::<Runtime>::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4])]);
@@ -196,8 +200,7 @@ mod pallet {
 			);
 
 			// any rebag is noop.
-			assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 1).is_ok()));
-			assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 10).is_ok()));
+			assert_storage_noop!(assert_eq!(BagsList::rebag(Origin::signed(0), 1), Ok(())));
 		})
 	}