From c4145831b104698ebe442e5e16c7ebee293e26a3 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Fri, 17 Sep 2021 16:43:24 -0700
Subject: [PATCH 01/30] pallet-bags-list: Add `put_in_front_of` extrinsic

This PR adds the extrinsic `put_in_front_of` which allows the user to specify a `lighter` and `heavier` account within the same bag. The extrinsic will move `heavier` directly in front of `lighter`.  The parameter names refer to the fact that `lighter` must have a lower `VoteWeight` then `heavier`.

In the ideal use case, where a user wants to improve the position of their account within a bag, the user would iterate the bag, starting from the head, and find the first node who's `VoteWeight` is less than theirs. They would then supply the `id` of the node as the `lighter` argument and their own `id` as the `heavier` argument.
---
 frame/bags-list/src/list/mod.rs | 80 ++++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 5 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 3f55f22271910..0791bc985fca7 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -41,6 +41,16 @@ use sp_std::{
 pub enum Error {
 	/// A duplicate id has been detected.
 	Duplicate,
+	/// Attempted to place node in front of a node in another bag.
+	NotInSameBag,
+	/// Id not found in list
+	IdNotFound,
+	/// Does not have a greater vote weight than the node it was placed in front of.
+	NotHeavier,
+	/// Higher weight node is already in a higher position than the the lesser weight node.
+	AlreadyHigherPosition,
+	/// Bag could not be found. This is a system logic error that should never happen.
+	BagNotFound,
 }
 
 #[cfg(test)]
@@ -290,11 +300,6 @@ impl<T: Config> List<T> {
 		Ok(())
 	}
 
-	/// Remove an id from the list.
-	pub(crate) fn remove(id: &T::AccountId) {
-		Self::remove_many(sp_std::iter::once(id));
-	}
-
 	/// Remove many ids from the list.
 	///
 	/// This is more efficient than repeated calls to `Self::remove`.
@@ -384,6 +389,71 @@ impl<T: Config> List<T> {
 		})
 	}
 
+	/// Mover `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in
+	/// same bag  and the `weight_of` `lighter_id` must be less than that of `heavier_id`
+	pub(crate) fn move_in_front_of(
+		lighter_id: &T::AccountId,
+		heavier_id: &T::AccountId,
+		weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
+	) -> Result<(), Error> {
+		use frame_support::ensure;
+
+		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(Error::IdNotFound)?;
+		let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(Error::IdNotFound)?;
+
+		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, Error::NotInSameBag);
+		ensure!(heavier_node.next.as_ref() != Some(lighter_id), Error::AlreadyHigherPosition);
+
+		// this is the most expensive check, so we do it last
+		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), Error::NotHeavier);
+
+		// check if the bag needs to be updated in storage
+		if lighter_node.is_terminal() || heavier_node.is_terminal() {
+			let mut bag = Bag::<T>::get(lighter_node.bag_upper).ok_or_else(|| {
+				debug_assert!(false, "bag that should exist cannot be found");
+				crate::log!(warn, "bag that should exist cannot be found");
+				Error::BagNotFound
+			})?;
+			debug_assert!(bag.iter().count() > 1);
+			if bag.head.as_ref() == Some(lighter_id) {
+				debug_assert!(lighter_node.next.is_some(), "lighter node must have next if head");
+				bag.head = Some(heavier_id.clone());
+			}
+			// re-assign bag tail if lighter is the tail
+			if bag.tail.as_ref() == Some(heavier_id) {
+				debug_assert!(heavier_node.prev.is_some(), "heaver node must have prev if tail");
+				bag.tail = heavier_node.prev.clone();
+			}
+			// we need to write the bag to storage if its head and/or tail is updated
+			bag.put()
+		}
+
+		// cut heavier out of the list, updating its neighbors
+		heavier_node.excise();
+
+		// connect heavier to its new prev
+		if let Some(mut prev) = lighter_node.prev() {
+			prev.next = Some(heavier_id.clone());
+			prev.put()
+		}
+		heavier_node.prev = lighter_node.prev;
+
+		// connect heavier and lighter
+		heavier_node.next = Some(lighter_id.clone());
+		lighter_node.prev = Some(heavier_id.clone());
+
+		// write the updated nodes to storage
+		lighter_node.put();
+		heavier_node.put();
+
+		Ok(())
+	}
+
+	/// Remove an id from the list.
+	pub(crate) fn remove(id: &T::AccountId) {
+		Self::remove_many(sp_std::iter::once(id));
+	}
+
 	/// Sanity check the list.
 	///
 	/// This should be called from the call-site, whenever one of the mutating apis (e.g. `insert`)

From 331df1bef0bade342e141fe0e3172bfae14f6a1f Mon Sep 17 00:00:00 2001
From: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Date: Mon, 20 Sep 2021 20:15:41 -0700
Subject: [PATCH 02/30] Test & Benchmarks

---
 frame/bags-list/src/benchmarks.rs |  38 +++++
 frame/bags-list/src/lib.rs        |  35 ++++
 frame/bags-list/src/list/mod.rs   |  59 +++----
 frame/bags-list/src/list/tests.rs | 256 ++++++++++++++++++++++++++++++
 frame/bags-list/src/mock.rs       |  29 ++--
 frame/bags-list/src/tests.rs      | 212 ++++++++++++++++++++++++-
 frame/bags-list/src/weights.rs    |   7 +
 7 files changed, 597 insertions(+), 39 deletions(-)

diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs
index a820eeba13b12..2d397f56a50cd 100644
--- a/frame/bags-list/src/benchmarks.rs
+++ b/frame/bags-list/src/benchmarks.rs
@@ -134,6 +134,44 @@ frame_benchmarking::benchmarks! {
 			]
 		);
 	}
+
+	put_in_front_of {
+		// The most expensive case for `put_in_front_of`:
+		//
+		// - both heavier's `prev` and `next` are nodes that will need to be read and written.
+		// - `lighter` is the bag's `head`, so the bag will need to be read and written.
+
+		let bag_thresh = T::BagThresholds::get()[0];
+
+		// insert the nodes in order
+		let lighter: T::AccountId = account("lighter", 0, 0);
+		assert_ok!(List::<T>::insert(lighter.clone(), bag_thresh));
+
+		let heavier_prev: T::AccountId = account("heavier_prev", 0, 0);
+		assert_ok!(List::<T>::insert(heavier_prev.clone(), bag_thresh));
+
+		let heavier: T::AccountId = account("heavier", 0, 0);
+		assert_ok!(List::<T>::insert(heavier.clone(), bag_thresh));
+
+		let heavier_next: T::AccountId = account("heavier_next", 0, 0);
+		assert_ok!(List::<T>::insert(heavier_next.clone(), bag_thresh));
+
+		T::VoteWeightProvider::set_vote_weight_of(&lighter, bag_thresh - 1);
+		T::VoteWeightProvider::set_vote_weight_of(&heavier, bag_thresh);
+
+		assert_eq!(
+			List::<T>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
+			vec![lighter.clone(), heavier_prev.clone(), heavier.clone(), heavier_next.clone()]
+		);
+
+		let caller = whitelisted_caller();
+	}: _(SystemOrigin::Signed(caller), lighter.clone(), heavier.clone())
+	verify {
+		assert_eq!(
+			List::<T>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
+			vec![heavier, lighter, heavier_prev, heavier_next]
+		)
+	}
 }
 
 use frame_benchmarking::impl_benchmark_test_suite;
diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 4202a4d499895..3380aed4767ff 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -178,6 +178,21 @@ pub mod pallet {
 		Rebagged(T::AccountId, VoteWeight, VoteWeight),
 	}
 
+	#[pallet::error]
+	#[cfg_attr(test, derive(PartialEq))]
+	pub enum Error<T> {
+		/// Attempted to place node in front of a node in another bag.
+		NotInSameBag,
+		/// Id not found in list.
+		IdNotFound,
+		/// An Id does not have a greater vote weight than another Id.
+		NotHeavier,
+		/// Heavier weight Id is already in a higher position than a lesser weight Id.
+		AlreadyHigherPosition,
+		/// Bag could not be found. This is a system logic error that should never happen.
+		BagNotFound,
+	}
+
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		/// Declare that some `dislocated` account has, through rewards or penalties, sufficiently
@@ -195,6 +210,26 @@ pub mod pallet {
 			let _ = Pallet::<T>::do_rebag(&dislocated, current_weight);
 			Ok(())
 		}
+
+		/// Move `heavier` directly in front of `lighter`.
+		///
+		/// Only works if
+		/// - both nodes are within the same bag,
+		/// - and `heavier` has a greater `VoteWeight` than `lighter`.
+		#[pallet::weight(T::WeightInfo::put_in_front_of())]
+		pub fn put_in_front_of(
+			origin: OriginFor<T>,
+			heavier: T::AccountId,
+			lighter: T::AccountId,
+		) -> DispatchResult {
+			ensure_signed(origin)?;
+			List::<T>::put_in_front_of(
+				&heavier,
+				&lighter,
+				Box::new(T::VoteWeightProvider::vote_weight),
+			)
+			.map_err(Into::into)
+		}
 	}
 
 	#[pallet::hooks]
diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 0791bc985fca7..8cfe599dc1616 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -41,16 +41,6 @@ use sp_std::{
 pub enum Error {
 	/// A duplicate id has been detected.
 	Duplicate,
-	/// Attempted to place node in front of a node in another bag.
-	NotInSameBag,
-	/// Id not found in list
-	IdNotFound,
-	/// Does not have a greater vote weight than the node it was placed in front of.
-	NotHeavier,
-	/// Higher weight node is already in a higher position than the the lesser weight node.
-	AlreadyHigherPosition,
-	/// Bag could not be found. This is a system logic error that should never happen.
-	BagNotFound,
 }
 
 #[cfg(test)]
@@ -300,6 +290,11 @@ impl<T: Config> List<T> {
 		Ok(())
 	}
 
+	/// Remove an id from the list.
+	pub(crate) fn remove(id: &T::AccountId) {
+		Self::remove_many(sp_std::iter::once(id));
+	}
+
 	/// Remove many ids from the list.
 	///
 	/// This is more efficient than repeated calls to `Self::remove`.
@@ -389,56 +384,67 @@ impl<T: Config> List<T> {
 		})
 	}
 
-	/// Mover `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in
+	/// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in
 	/// same bag  and the `weight_of` `lighter_id` must be less than that of `heavier_id`
-	pub(crate) fn move_in_front_of(
+	pub(crate) fn put_in_front_of(
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
 		weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
-	) -> Result<(), Error> {
+	) -> Result<(), crate::pallet::Error<T>> {
+		use crate::pallet;
 		use frame_support::ensure;
 
-		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(Error::IdNotFound)?;
-		let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(Error::IdNotFound)?;
+		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
+		let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;
 
-		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, Error::NotInSameBag);
-		ensure!(heavier_node.next.as_ref() != Some(lighter_id), Error::AlreadyHigherPosition);
+		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);
+		ensure!(
+			heavier_node.next.as_ref() != Some(lighter_id),
+			pallet::Error::AlreadyHigherPosition
+		);
 
 		// this is the most expensive check, so we do it last
-		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), Error::NotHeavier);
+		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
 
-		// check if the bag needs to be updated in storage
+		// we need to write the bag to storage if its head and/or tail is updated
 		if lighter_node.is_terminal() || heavier_node.is_terminal() {
 			let mut bag = Bag::<T>::get(lighter_node.bag_upper).ok_or_else(|| {
 				debug_assert!(false, "bag that should exist cannot be found");
 				crate::log!(warn, "bag that should exist cannot be found");
-				Error::BagNotFound
+				pallet::Error::BagNotFound
 			})?;
 			debug_assert!(bag.iter().count() > 1);
+
 			if bag.head.as_ref() == Some(lighter_id) {
 				debug_assert!(lighter_node.next.is_some(), "lighter node must have next if head");
 				bag.head = Some(heavier_id.clone());
 			}
+
 			// re-assign bag tail if lighter is the tail
 			if bag.tail.as_ref() == Some(heavier_id) {
-				debug_assert!(heavier_node.prev.is_some(), "heaver node must have prev if tail");
+				debug_assert!(heavier_node.prev.is_some(), "heavier node must have prev if tail");
 				bag.tail = heavier_node.prev.clone();
 			}
-			// we need to write the bag to storage if its head and/or tail is updated
+
+			// within this block we know the bag must have been updated, so we update storage
 			bag.put()
 		}
 
 		// cut heavier out of the list, updating its neighbors
 		heavier_node.excise();
 
-		// connect heavier to its new prev
+		// re-fetch `lighter_node` from storage since it may have been updated when heavier node was
+		// excised
+		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?; // TODO this should never fail, but if does could be bad news
+
+		// connect `heavier_node` to its new `prev`
 		if let Some(mut prev) = lighter_node.prev() {
 			prev.next = Some(heavier_id.clone());
 			prev.put()
 		}
 		heavier_node.prev = lighter_node.prev;
 
-		// connect heavier and lighter
+		// connect `heavier_node` and `lighter`
 		heavier_node.next = Some(lighter_id.clone());
 		lighter_node.prev = Some(heavier_id.clone());
 
@@ -449,11 +455,6 @@ impl<T: Config> List<T> {
 		Ok(())
 	}
 
-	/// Remove an id from the list.
-	pub(crate) fn remove(id: &T::AccountId) {
-		Self::remove_many(sp_std::iter::once(id));
-	}
-
 	/// Sanity check the list.
 	///
 	/// This should be called from the call-site, whenever one of the mutating apis (e.g. `insert`)
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index e2730cbf4e33d..d3534401a0f48 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -374,6 +374,262 @@ mod list {
 			assert!(non_existent_ids.iter().all(|id| !List::<Runtime>::contains(id)));
 		})
 	}
+
+	#[test]
+	fn put_in_front_of_with_two_node_bag() {
+		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// when
+			assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_with_non_terminal_nodes() {
+		ExtBuilder::default()
+			.add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+				// when
+				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_lighter_as_head() {
+		ExtBuilder::default()
+			.add_ids(vec![(710, 15), (711, 16), (12, 20)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+				// when
+				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_heavier_as_tail() {
+		ExtBuilder::default()
+			.add_ids(vec![(11, 20), (710, 15), (711, 16)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+				// when
+				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
+		ExtBuilder::default()
+			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 16)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+				// when
+				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_heavier_is_less_than_lighter() {
+		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&711, &710, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::NotHeavier
+			));
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_heavier_is_already_higher() {
+		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+			//	note insertion order ^^^^^^
+
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&711, &710, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::AlreadyHigherPosition
+			));
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_nodes_not_found() {
+		// `heavier` not found
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			assert!(!ListNodes::<Runtime>::contains_key(5));
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&4, &5, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::IdNotFound
+			));
+		});
+
+		// `lighter` not found
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			assert!(!ListNodes::<Runtime>::contains_key(5));
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&5, &4, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::IdNotFound
+			));
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_nodes_not_in_same_bag() {
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&1, &4, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::NotInSameBag
+			));
+		});
+	}
+
+	#[test]
+	#[should_panic(expected = "bag that should exist cannot be found")]
+	fn put_in_front_of_panics_if_bag_not_found() {
+		ExtBuilder::default().build_and_execute(|| {
+			let node_710_no_bag =
+				Node::<Runtime> { id: 710, prev: None, next: None, bag_upper: 15 };
+			let node_711_no_bag =
+				Node::<Runtime> { id: 711, prev: None, next: None, bag_upper: 15 };
+
+			// given
+			ListNodes::<Runtime>::insert(710, node_710_no_bag);
+			ListNodes::<Runtime>::insert(711, node_711_no_bag);
+			assert!(!ListBags::<Runtime>::contains_key(15));
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+			//      710 & 711 our not in reachable via iteration ^^^^^
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&710, &711, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::BagNotFound
+			));
+		});
+	}
 }
 
 mod bags {
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index a6ab35896b1e7..612e286b23c86 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -21,28 +21,34 @@ use super::*;
 use crate::{self as bags_list};
 use frame_election_provider_support::VoteWeight;
 use frame_support::parameter_types;
+use std::{cell::RefCell, collections::HashMap};
 
 pub type AccountId = u32;
 pub type Balance = u32;
 
+thread_local! {
+	static NEXT_VOTE_WEIGHT_MAP: RefCell<HashMap<AccountId, VoteWeight>> = RefCell::new(Default::default());
+}
+
 parameter_types! {
+	// Set the vote weight for any id who's weight has _not_ been set with `set_vote_weight_of`.
 	pub static NextVoteWeight: VoteWeight = 0;
 }
 
 pub struct StakingMock;
 impl frame_election_provider_support::VoteWeightProvider<AccountId> for StakingMock {
 	fn vote_weight(id: &AccountId) -> VoteWeight {
-		match id {
-			710 => 15,
-			711 => 16,
-			712 => 2_000, // special cases used for migrate test
-			_ => NextVoteWeight::get(),
-		}
+		NEXT_VOTE_WEIGHT_MAP.with(|m| {
+			m.borrow_mut()
+				.get(id)
+				.map(|weight| weight.clone())
+				.unwrap_or(NextVoteWeight::get())
+		})
 	}
+
 	#[cfg(any(feature = "runtime-benchmarks", test))]
-	fn set_vote_weight_of(_: &AccountId, weight: VoteWeight) {
-		// we don't really keep a mapping, just set weight for everyone.
-		NextVoteWeight::set(weight)
+	fn set_vote_weight_of(id: &AccountId, weight: VoteWeight) {
+		NEXT_VOTE_WEIGHT_MAP.with(|m| m.borrow_mut().insert(id.clone(), weight));
 	}
 }
 
@@ -116,6 +122,11 @@ impl ExtBuilder {
 		sp_tracing::try_init_simple();
 		let storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
 
+		// special cases used for `migrate` and `put_in_front_of` tests.
+		StakingMock::set_vote_weight_of(&710, 15);
+		StakingMock::set_vote_weight_of(&711, 16);
+		StakingMock::set_vote_weight_of(&712, 2_000);
+
 		let mut ext = sp_io::TestExternalities::from(storage);
 		ext.execute_with(|| {
 			for (id, weight) in GENESIS_IDS.iter().chain(self.ids.iter()) {
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index e94017730668b..cf61c2262dd30 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use frame_support::{assert_ok, assert_storage_noop, traits::IntegrityTest};
+use frame_support::{assert_noop, assert_ok, assert_storage_noop, traits::IntegrityTest};
 
 use super::*;
 use frame_election_provider_support::SortedListProvider;
@@ -196,6 +196,216 @@ mod pallet {
 			assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 10).is_ok()));
 		})
 	}
+
+	#[test]
+	fn put_in_front_of_with_two_node_bag() {
+		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			// when
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_with_non_terminal_nodes() {
+		ExtBuilder::default()
+			.add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_lighter_as_head() {
+		ExtBuilder::default()
+			.add_ids(vec![(710, 15), (711, 16), (12, 20)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_heavier_as_tail() {
+		ExtBuilder::default()
+			.add_ids(vec![(11, 20), (710, 15), (711, 16)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
+		ExtBuilder::default()
+			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 16)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])]
+				);
+
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])]
+				);
+			});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_heavier_is_less_than_lighter() {
+		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			// then
+			assert_noop!(
+				BagsList::put_in_front_of(Origin::signed(0), 711, 710),
+				crate::pallet::Error::<Runtime>::NotHeavier
+			);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_heavier_is_already_higher() {
+		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+			//	note insertion order ^^^^^^
+
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			// then
+			assert_noop!(
+				BagsList::put_in_front_of(Origin::signed(0), 711, 710),
+				crate::pallet::Error::<Runtime>::AlreadyHigherPosition
+			);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_nodes_not_found() {
+		// `heavier` not found
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			assert!(!ListNodes::<Runtime>::contains_key(5));
+
+			// then
+			assert_noop!(
+				BagsList::put_in_front_of(Origin::signed(0), 4, 5),
+				crate::pallet::Error::<Runtime>::IdNotFound
+			);
+		});
+
+		// `lighter` not found
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			assert!(!ListNodes::<Runtime>::contains_key(5));
+
+			// then
+			assert_noop!(
+				BagsList::put_in_front_of(Origin::signed(0), 5, 4),
+				crate::pallet::Error::<Runtime>::IdNotFound
+			);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_exits_early_if_nodes_not_in_same_bag() {
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			// then
+			assert_noop!(
+				BagsList::put_in_front_of(Origin::signed(0), 1, 4),
+				crate::pallet::Error::<Runtime>::NotInSameBag
+			);
+		});
+	}
 }
 
 mod sorted_list_provider {
diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index 95d3dfa6eb989..66bcbb7bf65d6 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -47,6 +47,7 @@ use sp_std::marker::PhantomData;
 pub trait WeightInfo {
 	fn rebag_non_terminal() -> Weight;
 	fn rebag_terminal() -> Weight;
+	fn put_in_front_of() -> Weight;
 }
 
 /// Weights for pallet_bags_list using the Substrate node and recommended hardware.
@@ -70,6 +71,9 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().reads(7 as Weight))
 			.saturating_add(T::DbWeight::get().writes(5 as Weight))
 	}
+	fn put_in_front_of() -> Weight {
+		0
+	}
 }
 
 // For backwards compatibility and tests
@@ -92,4 +96,7 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
 	}
+	fn put_in_front_of() -> Weight {
+		0
+	}
 }

From 2401b2b74ad52456b44c4ab1b65ffe52547d3234 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Mon, 20 Sep 2021 20:58:21 -0700
Subject: [PATCH 03/30] Respect line width

---
 frame/bags-list/src/list/mod.rs | 2 +-
 frame/bags-list/src/mock.rs     | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 8cfe599dc1616..16a8f74bbdf6c 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -435,7 +435,7 @@ impl<T: Config> List<T> {
 
 		// re-fetch `lighter_node` from storage since it may have been updated when heavier node was
 		// excised
-		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?; // TODO this should never fail, but if does could be bad news
+		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
 
 		// connect `heavier_node` to its new `prev`
 		if let Some(mut prev) = lighter_node.prev() {
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index 612e286b23c86..d26eb5aa0f3f5 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -27,7 +27,8 @@ pub type AccountId = u32;
 pub type Balance = u32;
 
 thread_local! {
-	static NEXT_VOTE_WEIGHT_MAP: RefCell<HashMap<AccountId, VoteWeight>> = RefCell::new(Default::default());
+	static NEXT_VOTE_WEIGHT_MAP: RefCell<HashMap<AccountId, VoteWeight>>
+		= RefCell::new(Default::default());
 }
 
 parameter_types! {

From 60743ccfce9cb8a85d8a817e009d4a3bb21ff771 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Tue, 21 Sep 2021 10:42:33 -0700
Subject: [PATCH 04/30] Remove List::put_in_fron_of tests; Remove AlreadyHigher
 error

---
 frame/bags-list/src/list/mod.rs   |   4 -
 frame/bags-list/src/list/tests.rs | 256 ------------------------------
 frame/bags-list/src/tests.rs      |  18 ++-
 3 files changed, 11 insertions(+), 267 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 16a8f74bbdf6c..0ea3a899a0790 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -398,10 +398,6 @@ impl<T: Config> List<T> {
 		let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;
 
 		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);
-		ensure!(
-			heavier_node.next.as_ref() != Some(lighter_id),
-			pallet::Error::AlreadyHigherPosition
-		);
 
 		// this is the most expensive check, so we do it last
 		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index d3534401a0f48..e2730cbf4e33d 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -374,262 +374,6 @@ mod list {
 			assert!(non_existent_ids.iter().all(|id| !List::<Runtime>::contains(id)));
 		})
 	}
-
-	#[test]
-	fn put_in_front_of_with_two_node_bag() {
-		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
-			);
-
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// when
-			assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
-
-			// then
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
-			);
-		});
-	}
-
-	#[test]
-	fn put_in_front_of_with_non_terminal_nodes() {
-		ExtBuilder::default()
-			.add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-				// when
-				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
-	}
-
-	#[test]
-	fn put_in_front_of_with_lighter_as_head() {
-		ExtBuilder::default()
-			.add_ids(vec![(710, 15), (711, 16), (12, 20)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-				// when
-				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
-	}
-
-	#[test]
-	fn put_in_front_of_with_heavier_as_tail() {
-		ExtBuilder::default()
-			.add_ids(vec![(11, 20), (710, 15), (711, 16)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-				// when
-				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])]
-				);
-			});
-	}
-
-	#[test]
-	fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
-		ExtBuilder::default()
-			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 16)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-				// when
-				assert_ok!(List::<Runtime>::put_in_front_of(&710, &711, weight_fn));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
-	}
-
-	#[test]
-	fn put_in_front_of_exits_early_if_heavier_is_less_than_lighter() {
-		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
-			);
-
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&711, &710, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::NotHeavier
-			));
-		});
-	}
-
-	#[test]
-	fn put_in_front_of_exits_early_if_heavier_is_already_higher() {
-		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
-			//	note insertion order ^^^^^^
-
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
-			);
-
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&711, &710, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::AlreadyHigherPosition
-			));
-		});
-	}
-
-	#[test]
-	fn put_in_front_of_exits_early_if_nodes_not_found() {
-		// `heavier` not found
-		ExtBuilder::default().build_and_execute(|| {
-			// given
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-
-			assert!(!ListNodes::<Runtime>::contains_key(5));
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&4, &5, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::IdNotFound
-			));
-		});
-
-		// `lighter` not found
-		ExtBuilder::default().build_and_execute(|| {
-			// given
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-
-			assert!(!ListNodes::<Runtime>::contains_key(5));
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&5, &4, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::IdNotFound
-			));
-		});
-	}
-
-	#[test]
-	fn put_in_front_of_exits_early_if_nodes_not_in_same_bag() {
-		ExtBuilder::default().build_and_execute(|| {
-			// given
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&1, &4, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::NotInSameBag
-			));
-		});
-	}
-
-	#[test]
-	#[should_panic(expected = "bag that should exist cannot be found")]
-	fn put_in_front_of_panics_if_bag_not_found() {
-		ExtBuilder::default().build_and_execute(|| {
-			let node_710_no_bag =
-				Node::<Runtime> { id: 710, prev: None, next: None, bag_upper: 15 };
-			let node_711_no_bag =
-				Node::<Runtime> { id: 711, prev: None, next: None, bag_upper: 15 };
-
-			// given
-			ListNodes::<Runtime>::insert(710, node_710_no_bag);
-			ListNodes::<Runtime>::insert(711, node_711_no_bag);
-			assert!(!ListBags::<Runtime>::contains_key(15));
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-			//      710 & 711 our not in reachable via iteration ^^^^^
-
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&710, &711, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::BagNotFound
-			));
-		});
-	}
 }
 
 mod bags {
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index cf61c2262dd30..4855cdb52de22 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -341,23 +341,27 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_exits_early_if_heavier_is_already_higher() {
-		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+	fn put_in_front_of_exits_early_if_heavier_is_equal_weight_to_lighter() {
+		use mock::StakingMock;
+
+		ExtBuilder::default().add_ids(vec![(5, 15), (6, 15)]).build_and_execute(|| {
 			//	note insertion order ^^^^^^
 
 			// given
 			assert_eq!(
 				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+				vec![(10, vec![1]), (20, vec![5, 6]), (1_000, vec![2, 3, 4])]
 			);
 
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+			StakingMock::set_vote_weight_of(&5, 15);
+			StakingMock::set_vote_weight_of(&6, 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&5), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&6), 15);
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 711, 710),
-				crate::pallet::Error::<Runtime>::AlreadyHigherPosition
+				BagsList::put_in_front_of(Origin::signed(0), 5, 6),
+				crate::pallet::Error::<Runtime>::NotHeavier
 			);
 		});
 	}

From 6bb232f16cc31a4c6324917b4b343bd8b3586262 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Tue, 21 Sep 2021 10:52:35 -0700
Subject: [PATCH 05/30] The dispatch origin for this call must be ...

---
 frame/bags-list/src/lib.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 3380aed4767ff..f22c884a1594d 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -213,6 +213,8 @@ pub mod pallet {
 
 		/// Move `heavier` directly in front of `lighter`.
 		///
+		/// The dispatch origin for this call must be _Signed_ and can be called by anyone.
+		///
 		/// Only works if
 		/// - both nodes are within the same bag,
 		/// - and `heavier` has a greater `VoteWeight` than `lighter`.

From 45f4a73e8c15ef9922bd9a961e554303918525ca Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Tue, 21 Sep 2021 10:56:47 -0700
Subject: [PATCH 06/30] Add some periods

---
 frame/bags-list/src/list/mod.rs | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 0ea3a899a0790..3d96089535531 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -385,7 +385,7 @@ impl<T: Config> List<T> {
 	}
 
 	/// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in
-	/// same bag  and the `weight_of` `lighter_id` must be less than that of `heavier_id`
+	/// same bag  and the `weight_of` `lighter_id` must be less than that of `heavier_id`.
 	pub(crate) fn put_in_front_of(
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
@@ -399,10 +399,10 @@ impl<T: Config> List<T> {
 
 		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);
 
-		// this is the most expensive check, so we do it last
+		// this is the most expensive check, so we do it last.
 		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
 
-		// we need to write the bag to storage if its head and/or tail is updated
+		// we need to write the bag to storage if its head and/or tail is updated.
 		if lighter_node.is_terminal() || heavier_node.is_terminal() {
 			let mut bag = Bag::<T>::get(lighter_node.bag_upper).ok_or_else(|| {
 				debug_assert!(false, "bag that should exist cannot be found");
@@ -422,29 +422,29 @@ impl<T: Config> List<T> {
 				bag.tail = heavier_node.prev.clone();
 			}
 
-			// within this block we know the bag must have been updated, so we update storage
+			// within this block we know the bag must have been updated, so we update storage.
 			bag.put()
 		}
 
-		// cut heavier out of the list, updating its neighbors
+		// cut heavier out of the list, updating its neighbors.
 		heavier_node.excise();
 
-		// re-fetch `lighter_node` from storage since it may have been updated when heavier node was
-		// excised
+		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` was
+		// excised.
 		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
 
-		// connect `heavier_node` to its new `prev`
+		// connect `heavier_node` to its new `prev`.
 		if let Some(mut prev) = lighter_node.prev() {
 			prev.next = Some(heavier_id.clone());
 			prev.put()
 		}
 		heavier_node.prev = lighter_node.prev;
 
-		// connect `heavier_node` and `lighter`
+		// connect `heavier_node` and `lighter`.
 		heavier_node.next = Some(lighter_id.clone());
 		lighter_node.prev = Some(heavier_id.clone());
 
-		// write the updated nodes to storage
+		// write the updated nodes to storage.
 		lighter_node.put();
 		heavier_node.put();
 

From f04a64c083fc39257a39438e9a418f4941af21be Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Tue, 21 Sep 2021 11:06:01 -0700
Subject: [PATCH 07/30] Add back test to list module:
 put_in_front_of_exits_early_if_bag_not_found

---
 frame/bags-list/src/list/tests.rs | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index e2730cbf4e33d..d7cbfdc67dcbf 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -374,6 +374,35 @@ mod list {
 			assert!(non_existent_ids.iter().all(|id| !List::<Runtime>::contains(id)));
 		})
 	}
+
+	#[test]
+	#[cfg_attr(
+		debug_assertions,
+		should_panic = "bag that should exist cannot be found"
+	)]
+	fn put_in_front_of_exits_early_if_bag_not_found() {
+		ExtBuilder::default().build_and_execute_no_post_check(|| {
+			let node_710_no_bag =
+				Node::<Runtime> { id: 710, prev: None, next: None, bag_upper: 15 };
+			let node_711_no_bag =
+				Node::<Runtime> { id: 711, prev: None, next: None, bag_upper: 15 };
+
+			// given
+			ListNodes::<Runtime>::insert(710, node_710_no_bag);
+			ListNodes::<Runtime>::insert(711, node_711_no_bag);
+			assert!(!ListBags::<Runtime>::contains_key(15));
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+			//      710 & 711 our not in reachable via iteration ^^^^^
+
+			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
+
+			// then
+			assert_storage_noop!(assert_eq!(
+				List::<Runtime>::put_in_front_of(&710, &711, weight_fn).unwrap_err(),
+				crate::pallet::Error::<Runtime>::BagNotFound
+			));
+		});
+	}
 }
 
 mod bags {

From aa0aaeb90eda8de26365e11af6b50381a045c660 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 22 Sep 2021 12:37:30 -0700
Subject: [PATCH 08/30] add test
 tests::pallet::heavier_is_head_lighter_is_not_terminal

---
 frame/bags-list/src/tests.rs | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index 4855cdb52de22..a3ba9596cc0e3 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -298,7 +298,7 @@ mod pallet {
 	#[test]
 	fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
 		ExtBuilder::default()
-			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 16)])
+			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 15)])
 			.build_and_execute(|| {
 				// given
 				assert_eq!(
@@ -321,7 +321,24 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_exits_early_if_heavier_is_less_than_lighter() {
+	fn heavier_is_head_lighter_is_not_terminal() {
+		ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
+
+			StakingMock::set_vote_weight_of(&4, 1_000);
+			StakingMock::set_vote_weight_of(&2, 999);
+
+			// when
+			BagsList::put_in_front_of(Origin::signed(0), 2, 4).unwrap();
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_errors_if_heavier_is_less_than_lighter() {
 		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
 			// given
 			assert_eq!(
@@ -341,7 +358,7 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_exits_early_if_heavier_is_equal_weight_to_lighter() {
+	fn put_in_front_of_errors_if_heavier_is_equal_weight_to_lighter() {
 		use mock::StakingMock;
 
 		ExtBuilder::default().add_ids(vec![(5, 15), (6, 15)]).build_and_execute(|| {
@@ -367,7 +384,7 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_exits_early_if_nodes_not_found() {
+	fn put_in_front_of_errors_if_nodes_not_found() {
 		// `heavier` not found
 		ExtBuilder::default().build_and_execute(|| {
 			// given
@@ -398,7 +415,7 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_exits_early_if_nodes_not_in_same_bag() {
+	fn put_in_front_of_errors_if_nodes_not_in_same_bag() {
 		ExtBuilder::default().build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);

From fa1a33d3fa9fdc3a288413ea388d1239ad8f0b7d Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 22 Sep 2021 20:32:32 -0700
Subject: [PATCH 09/30] Cater for edge case of heavier being head

---
 frame/bags-list/src/list/mod.rs   |  85 +++++++-----
 frame/bags-list/src/list/tests.rs |   5 +-
 frame/bags-list/src/tests.rs      | 209 +++++++++++++++++-------------
 3 files changed, 169 insertions(+), 130 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 3d96089535531..0fb5474e1bea7 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -395,58 +395,75 @@ impl<T: Config> List<T> {
 		use frame_support::ensure;
 
 		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
-		let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;
+		let heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;
 
 		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);
 
 		// this is the most expensive check, so we do it last.
 		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
 
-		// we need to write the bag to storage if its head and/or tail is updated.
-		if lighter_node.is_terminal() || heavier_node.is_terminal() {
-			let mut bag = Bag::<T>::get(lighter_node.bag_upper).ok_or_else(|| {
-				debug_assert!(false, "bag that should exist cannot be found");
-				crate::log!(warn, "bag that should exist cannot be found");
-				pallet::Error::BagNotFound
-			})?;
-			debug_assert!(bag.iter().count() > 1);
+		// remove the heavier node from this list. Note that this removes the node from storage and
+		// decrements the node counter.
+		Self::remove(&heavier_id);
 
-			if bag.head.as_ref() == Some(lighter_id) {
-				debug_assert!(lighter_node.next.is_some(), "lighter node must have next if head");
-				bag.head = Some(heavier_id.clone());
-			}
+		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
+		// was removed.
+		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
 
-			// re-assign bag tail if lighter is the tail
-			if bag.tail.as_ref() == Some(heavier_id) {
-				debug_assert!(heavier_node.prev.is_some(), "heavier node must have prev if tail");
-				bag.tail = heavier_node.prev.clone();
-			}
+		Self::insert_at(lighter_node, heavier_node)?;
 
-			// within this block we know the bag must have been updated, so we update storage.
-			bag.put()
-		}
+		// since `insert_at` was successful, increment the counter, which got decremented by
+		// `Self::remove`.
+		crate::CounterForListNodes::<T>::mutate(|prev_count| {
+			*prev_count = prev_count.saturating_add(1)
+		});
 
-		// cut heavier out of the list, updating its neighbors.
-		heavier_node.excise();
+		Ok(())
+	}
 
-		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` was
-		// excised.
-		let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
+	/// Insert `node` directly in front of `at`.
+	fn insert_at(mut at: Node<T>, mut node: Node<T>) -> Result<(), crate::pallet::Error<T>> {
+		use crate::pallet;
 
 		// connect `heavier_node` to its new `prev`.
-		if let Some(mut prev) = lighter_node.prev() {
-			prev.next = Some(heavier_id.clone());
+		node.prev = at.prev.clone();
+		if let Some(mut prev) = at.prev() {
+			prev.next = Some(node.id().clone());
 			prev.put()
 		}
-		heavier_node.prev = lighter_node.prev;
 
-		// connect `heavier_node` and `lighter`.
-		heavier_node.next = Some(lighter_id.clone());
-		lighter_node.prev = Some(heavier_id.clone());
+		// connect `node` and `at`.
+		node.next = Some(at.id().clone());
+		at.prev = Some(node.id().clone());
+
+		if at.is_terminal() || node.is_terminal() {
+			// `at` is the tail and/or `node` is the head, so we make sure the bag is updated. Note,
+			// since `node` is always in front of `at` we know that 1) there is always at least 2
+			// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
+			// tail.
+			let mut bag = Bag::<T>::get(at.bag_upper).ok_or_else(|| {
+				debug_assert!(false, "bag that should exist cannot be found");
+				crate::log!(warn, "bag that should exist cannot be found");
+				pallet::Error::BagNotFound
+			})?;
+
+			if node.prev == None {
+				bag.head = Some(node.id().clone())
+			}
+
+			if at.next == None {
+				bag.tail = Some(at.id().clone())
+			}
+
+			debug_assert!(at.prev.is_some(), "`at` cannot be the head");
+			debug_assert!(node.next.is_some(), "`node` cannot be the tail");
+
+			bag.put()
+		};
 
 		// write the updated nodes to storage.
-		lighter_node.put();
-		heavier_node.put();
+		at.put();
+		node.put();
 
 		Ok(())
 	}
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index afa66b62c1b47..b91bc07668699 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -376,10 +376,7 @@ mod list {
 	}
 
 	#[test]
-	#[cfg_attr(
-		debug_assertions,
-		should_panic = "bag that should exist cannot be found"
-	)]
+	#[cfg_attr(debug_assertions, should_panic = "bag that should exist cannot be found")]
 	fn put_in_front_of_exits_early_if_bag_not_found() {
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			let node_710_no_bag =
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index a3ba9596cc0e3..39f16220b1ffc 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -198,7 +198,7 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_with_two_node_bag() {
+	fn put_in_front_of_two_node_bag() {
 		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
 			// given
 			assert_eq!(
@@ -221,122 +221,147 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_with_non_terminal_nodes() {
-		ExtBuilder::default()
-			.add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				// when
-				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
+	fn put_in_front_of_two_node_bag_heavier_is_head() {
+		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
+			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+
+			// when
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+			);
+		});
 	}
 
 	#[test]
-	fn put_in_front_of_with_lighter_as_head() {
-		ExtBuilder::default()
-			.add_ids(vec![(710, 15), (711, 16), (12, 20)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				// when
-				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
+	fn put_in_front_of_head_and_tail_non_terminal_nodes() {
+		ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
+
+			StakingMock::set_vote_weight_of(&4, 1_000);
+			StakingMock::set_vote_weight_of(&3, 999);
+
+			// when
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
+		});
 	}
 
 	#[test]
-	fn put_in_front_of_with_heavier_as_tail() {
-		ExtBuilder::default()
-			.add_ids(vec![(11, 20), (710, 15), (711, 16)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				// when
-				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])]
-				);
-			});
+	fn put_in_front_of_lighter_is_head_heavier_is_non_terminal() {
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			StakingMock::set_vote_weight_of(&3, 1_000);
+			StakingMock::set_vote_weight_of(&2, 999);
+
+			// when
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 3));
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
+		});
 	}
 
 	#[test]
-	fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
-		ExtBuilder::default()
-			.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 15)])
-			.build_and_execute(|| {
-				// given
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])]
-				);
-
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-				assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
-
-				// when
-				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
-
-				// then
-				assert_eq!(
-					List::<Runtime>::get_bags(),
-					vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])]
-				);
-			});
+	fn put_in_front_of_heavier_is_tail_lighter_is_non_terminal() {
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			StakingMock::set_vote_weight_of(&4, 1_000);
+			StakingMock::set_vote_weight_of(&3, 999);
+
+			// when
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3])]);
+		});
 	}
 
 	#[test]
-	fn heavier_is_head_lighter_is_not_terminal() {
+	fn put_in_front_of_heavier_is_tail_lighter_is_head() {
 		ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
 
-			StakingMock::set_vote_weight_of(&4, 1_000);
+			StakingMock::set_vote_weight_of(&5, 1_000);
 			StakingMock::set_vote_weight_of(&2, 999);
 
 			// when
-			BagsList::put_in_front_of(Origin::signed(0), 2, 4).unwrap();
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 5));
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![5, 2, 3, 4])]);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_heavier_is_head_lighter_is_not_terminal() {
+		ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
+
+			StakingMock::set_vote_weight_of(&2, 1_000);
+			StakingMock::set_vote_weight_of(&4, 999);
+
+			// when
+			BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]);
 		});
 	}
 
+	#[test]
+	fn put_in_front_of_lighter_is_tail_heavier_is_not_terminal() {
+		ExtBuilder::default().add_ids(vec![(5, 900)]).build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
+
+			StakingMock::set_vote_weight_of(&5, 900);
+			StakingMock::set_vote_weight_of(&3, 1_000);
+
+			// when
+			BagsList::put_in_front_of(Origin::signed(0), 5, 3).unwrap();
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
+		});
+	}
+
+	#[test]
+	fn put_in_front_of_lighter_is_tail_heavier_is_head() {
+		ExtBuilder::default().build_and_execute(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			StakingMock::set_vote_weight_of(&2, 1_000);
+			StakingMock::set_vote_weight_of(&4, 999);
+
+			// when
+			BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();
+
+			// then
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
+		});
+	}
+
 	#[test]
 	fn put_in_front_of_errors_if_heavier_is_less_than_lighter() {
 		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {

From 21aef98315ad14a36d380fa44dc9ec51b00887b4 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 23 Sep 2021 00:54:15 -0700
Subject: [PATCH 10/30] Add ExtBuilder::add_aux_data; try to make some tests
 use simpler data

---
 frame/bags-list/src/list/mod.rs   |  3 --
 frame/bags-list/src/list/tests.rs | 76 ++++++++++++++-----------------
 frame/bags-list/src/mock.rs       | 15 ++++--
 frame/bags-list/src/tests.rs      | 66 ++++++++++++++++++---------
 4 files changed, 88 insertions(+), 72 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 0fb5474e1bea7..932ef24084e9c 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -455,9 +455,6 @@ impl<T: Config> List<T> {
 				bag.tail = Some(at.id().clone())
 			}
 
-			debug_assert!(at.prev.is_some(), "`at` cannot be the head");
-			debug_assert!(node.next.is_some(), "`node` cannot be the tail");
-
 			bag.put()
 		};
 
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index b91bc07668699..5eff1eebce601 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -109,42 +109,34 @@ fn remove_last_node_in_bags_cleans_bag() {
 
 #[test]
 fn migrate_works() {
-	ExtBuilder::default()
-		.add_ids(vec![(710, 15), (711, 16), (712, 2_000)])
-		.build_and_execute(|| {
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![
-					(10, vec![1]),
-					(20, vec![710, 711]),
-					(1_000, vec![2, 3, 4]),
-					(2_000, vec![712])
-				]
-			);
-			let old_thresholds = <Runtime as Config>::BagThresholds::get();
-			assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]);
+	ExtBuilder::default().add_aux_data().build_and_execute(|| {
+		// given
+		assert_eq!(
+			List::<Runtime>::get_bags(),
+			vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4]), (2_000, vec![712])]
+		);
+		let old_thresholds = <Runtime as Config>::BagThresholds::get();
+		assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]);
 
-			// when the new thresholds adds `15` and removes `2_000`
-			const NEW_THRESHOLDS: &'static [VoteWeight] =
-				&[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000];
-			BagThresholds::set(NEW_THRESHOLDS);
-			// and we call
-			List::<Runtime>::migrate(old_thresholds);
+		// when the new thresholds adds `15` and removes `2_000`
+		const NEW_THRESHOLDS: &'static [VoteWeight] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000];
+		BagThresholds::set(NEW_THRESHOLDS);
+		// and we call
+		List::<Runtime>::migrate(old_thresholds);
 
-			// then
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![
-					(10, vec![1]),
-					(15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15
-					(20, vec![711]),
-					(1_000, vec![2, 3, 4]),
-					// nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000
-					(10_000, vec![712]),
-				]
-			);
-		});
+		// then
+		assert_eq!(
+			List::<Runtime>::get_bags(),
+			vec![
+				(10, vec![1]),
+				(15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15
+				(20, vec![711]),
+				(1_000, vec![2, 3, 4]),
+				// nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000
+				(10_000, vec![712]),
+			]
+		);
+	});
 }
 
 mod list {
@@ -379,23 +371,23 @@ mod list {
 	#[cfg_attr(debug_assertions, should_panic = "bag that should exist cannot be found")]
 	fn put_in_front_of_exits_early_if_bag_not_found() {
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
-			let node_710_no_bag =
-				Node::<Runtime> { id: 710, prev: None, next: None, bag_upper: 15 };
-			let node_711_no_bag =
-				Node::<Runtime> { id: 711, prev: None, next: None, bag_upper: 15 };
+			let node_10_no_bag = Node::<Runtime> { id: 10, prev: None, next: None, bag_upper: 15 };
+			let node_11_no_bag = Node::<Runtime> { id: 11, prev: None, next: None, bag_upper: 15 };
 
 			// given
-			ListNodes::<Runtime>::insert(710, node_710_no_bag);
-			ListNodes::<Runtime>::insert(711, node_711_no_bag);
+			ListNodes::<Runtime>::insert(10, node_10_no_bag);
+			ListNodes::<Runtime>::insert(11, node_11_no_bag);
+			StakingMock::set_vote_weight_of(&10, 14);
+			StakingMock::set_vote_weight_of(&11, 15);
 			assert!(!ListBags::<Runtime>::contains_key(15));
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-			//      710 & 711 our not in reachable via iteration ^^^^^
+			//      10 & 11 our not in reachable via iteration ^^^^^
 
 			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
 
 			// then
 			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&710, &711, weight_fn).unwrap_err(),
+				List::<Runtime>::put_in_front_of(&10, &11, weight_fn).unwrap_err(),
 				crate::pallet::Error::<Runtime>::BagNotFound
 			));
 		});
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index d26eb5aa0f3f5..d33c7fd857b5b 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -119,14 +119,19 @@ impl ExtBuilder {
 		self
 	}
 
-	pub(crate) fn build(self) -> sp_io::TestExternalities {
-		sp_tracing::try_init_simple();
-		let storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
-
-		// special cases used for `migrate` and `put_in_front_of` tests.
+	/// Set vote weights of some Id's used for special case
+	pub(crate) fn add_aux_data(mut self) -> Self {
 		StakingMock::set_vote_weight_of(&710, 15);
 		StakingMock::set_vote_weight_of(&711, 16);
 		StakingMock::set_vote_weight_of(&712, 2_000);
+		self.ids.extend([(710, 15), (711, 16), (712, 2_000)].iter());
+
+		self
+	}
+
+	pub(crate) fn build(self) -> sp_io::TestExternalities {
+		sp_tracing::try_init_simple();
+		let storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
 
 		let mut ext = sp_io::TestExternalities::from(storage);
 		ext.execute_with(|| {
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index 39f16220b1ffc..ccbad2fa8c6e0 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -198,53 +198,53 @@ mod pallet {
 	}
 
 	#[test]
-	fn put_in_front_of_two_node_bag() {
-		ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
+	fn put_in_front_of_two_node_bag_heavier_is_tail() {
+		ExtBuilder::default().add_ids(vec![(10, 15), (11, 16)]).build_and_execute(|| {
 			// given
 			assert_eq!(
 				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4])]
+				vec![(10, vec![1]), (20, vec![10, 11]), (1_000, vec![2, 3, 4])]
 			);
 
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+			StakingMock::set_vote_weight_of(&10, 15);
+			StakingMock::set_vote_weight_of(&11, 16);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 10, 11));
 
 			// then
 			assert_eq!(
 				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
 			);
 		});
 	}
 
 	#[test]
 	fn put_in_front_of_two_node_bag_heavier_is_head() {
-		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
+		ExtBuilder::default().add_ids(vec![(11, 16), (10, 15)]).build_and_execute(|| {
 			// given
 			assert_eq!(
 				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
 			);
 
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+			StakingMock::set_vote_weight_of(&11, 16);
+			StakingMock::set_vote_weight_of(&10, 15);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 10, 11));
 
 			// then
 			assert_eq!(
 				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
+				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
 			);
 		});
 	}
 
 	#[test]
-	fn put_in_front_of_head_and_tail_non_terminal_nodes() {
+	fn put_in_front_of_non_terminal_nodes_heavier_behind() {
 		ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
@@ -260,6 +260,31 @@ mod pallet {
 		});
 	}
 
+	#[test]
+	fn put_in_front_of_non_terminal_nodes_heavier_in_front() {
+		ExtBuilder::default()
+			.add_ids(vec![(5, 1_000), (6, 1_000)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5, 6])]
+				);
+
+				StakingMock::set_vote_weight_of(&5, 999);
+				StakingMock::set_vote_weight_of(&3, 1_000);
+
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 5, 3));
+
+				// then
+				assert_eq!(
+					List::<Runtime>::get_bags(),
+					vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5, 6])]
+				);
+			});
+	}
+
 	#[test]
 	fn put_in_front_of_lighter_is_head_heavier_is_non_terminal() {
 		ExtBuilder::default().build_and_execute(|| {
@@ -364,19 +389,16 @@ mod pallet {
 
 	#[test]
 	fn put_in_front_of_errors_if_heavier_is_less_than_lighter() {
-		ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
+		ExtBuilder::default().build_and_execute(|| {
 			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
-			);
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);
+			StakingMock::set_vote_weight_of(&3, 999);
+			StakingMock::set_vote_weight_of(&2, 1_000);
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 711, 710),
+				BagsList::put_in_front_of(Origin::signed(0), 2, 3),
 				crate::pallet::Error::<Runtime>::NotHeavier
 			);
 		});

From 7433cf664bcc65e8feb19a70a1c73d6ab340daa2 Mon Sep 17 00:00:00 2001
From: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 23 Sep 2021 01:03:44 -0700
Subject: [PATCH 11/30] Update frame/bags-list/src/list/tests.rs

---
 frame/bags-list/src/list/tests.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index 5eff1eebce601..2f5fbc18ce6cb 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -381,7 +381,6 @@ mod list {
 			StakingMock::set_vote_weight_of(&11, 15);
 			assert!(!ListBags::<Runtime>::contains_key(15));
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
-			//      10 & 11 our not in reachable via iteration ^^^^^
 
 			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
 

From 1760815bc71f099bbcc0159e7af51316405b3823 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 23 Sep 2021 10:46:10 -0700
Subject: [PATCH 12/30] make insert_at_unchecked infallible

---
 frame/bags-list/src/list/mod.rs   | 23 +++++++++--------------
 frame/bags-list/src/list/tests.rs |  4 ++--
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 932ef24084e9c..e9e8455c2385b 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -409,11 +409,10 @@ impl<T: Config> List<T> {
 		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
 		// was removed.
 		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
+		Self::insert_at_unchecked(lighter_node, heavier_node);
 
-		Self::insert_at(lighter_node, heavier_node)?;
-
-		// since `insert_at` was successful, increment the counter, which got decremented by
-		// `Self::remove`.
+		// Increment the counter, which got decremented by `Self::remove`, since we just re-inserted
+		// `heavier_node`.
 		crate::CounterForListNodes::<T>::mutate(|prev_count| {
 			*prev_count = prev_count.saturating_add(1)
 		});
@@ -422,9 +421,10 @@ impl<T: Config> List<T> {
 	}
 
 	/// Insert `node` directly in front of `at`.
-	fn insert_at(mut at: Node<T>, mut node: Node<T>) -> Result<(), crate::pallet::Error<T>> {
-		use crate::pallet;
-
+	///
+	/// WARNING: we do not check that `node` or `at` are valid. This will panic if `at.bag_upper` is
+	/// not a bag that already exists in storage.
+	fn insert_at_unchecked(mut at: Node<T>, mut node: Node<T>) {
 		// connect `heavier_node` to its new `prev`.
 		node.prev = at.prev.clone();
 		if let Some(mut prev) = at.prev() {
@@ -441,11 +441,8 @@ impl<T: Config> List<T> {
 			// since `node` is always in front of `at` we know that 1) there is always at least 2
 			// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
 			// tail.
-			let mut bag = Bag::<T>::get(at.bag_upper).ok_or_else(|| {
-				debug_assert!(false, "bag that should exist cannot be found");
-				crate::log!(warn, "bag that should exist cannot be found");
-				pallet::Error::BagNotFound
-			})?;
+			let mut bag =
+				Bag::<T>::get(at.bag_upper).expect("given nodes must always have a valid. qed.");
 
 			if node.prev == None {
 				bag.head = Some(node.id().clone())
@@ -461,8 +458,6 @@ impl<T: Config> List<T> {
 		// write the updated nodes to storage.
 		at.put();
 		node.put();
-
-		Ok(())
 	}
 
 	/// Sanity check the list.
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index 5eff1eebce601..ceea5b4da2748 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -368,8 +368,8 @@ mod list {
 	}
 
 	#[test]
-	#[cfg_attr(debug_assertions, should_panic = "bag that should exist cannot be found")]
-	fn put_in_front_of_exits_early_if_bag_not_found() {
+	#[should_panic = "given nodes must always have a valid. qed."]
+	fn put_in_front_of_panics_if_bag_not_found() {
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			let node_10_no_bag = Node::<Runtime> { id: 10, prev: None, next: None, bag_upper: 15 };
 			let node_11_no_bag = Node::<Runtime> { id: 11, prev: None, next: None, bag_upper: 15 };

From afff2a9c5c4fc71793015102f4a5f797783f9798 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 23 Sep 2021 11:12:56 -0700
Subject: [PATCH 13/30] Make it permissioned - only callable by heavier

---
 frame/bags-list/src/benchmarks.rs |  6 +++---
 frame/bags-list/src/lib.rs        | 17 +++++++----------
 frame/bags-list/src/tests.rs      | 30 +++++++++++++++---------------
 3 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs
index 2d397f56a50cd..e68024463dd68 100644
--- a/frame/bags-list/src/benchmarks.rs
+++ b/frame/bags-list/src/benchmarks.rs
@@ -19,7 +19,7 @@
 
 use super::*;
 use crate::list::List;
-use frame_benchmarking::{account, whitelisted_caller};
+use frame_benchmarking::{account, whitelist_account, whitelisted_caller};
 use frame_election_provider_support::VoteWeightProvider;
 use frame_support::{assert_ok, traits::Get};
 use frame_system::RawOrigin as SystemOrigin;
@@ -164,8 +164,8 @@ frame_benchmarking::benchmarks! {
 			vec![lighter.clone(), heavier_prev.clone(), heavier.clone(), heavier_next.clone()]
 		);
 
-		let caller = whitelisted_caller();
-	}: _(SystemOrigin::Signed(caller), lighter.clone(), heavier.clone())
+		whitelist_account!(heavier);
+	}: _(SystemOrigin::Signed(heavier.clone()), lighter.clone())
 	verify {
 		assert_eq!(
 			List::<T>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index f22c884a1594d..b6aeb6b9b085e 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -211,23 +211,20 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// Move `heavier` directly in front of `lighter`.
+		/// Move the caller's Id directly in front of `lighter`.
 		///
-		/// The dispatch origin for this call must be _Signed_ and can be called by anyone.
+		/// The dispatch origin for this call must be _Signed_ and can only be called by the Id of
+		/// the account going in front of `lighter`.
 		///
 		/// Only works if
 		/// - both nodes are within the same bag,
-		/// - and `heavier` has a greater `VoteWeight` than `lighter`.
+		/// - and `origin` has a greater `VoteWeight` than `lighter`.
 		#[pallet::weight(T::WeightInfo::put_in_front_of())]
-		pub fn put_in_front_of(
-			origin: OriginFor<T>,
-			heavier: T::AccountId,
-			lighter: T::AccountId,
-		) -> DispatchResult {
-			ensure_signed(origin)?;
+		pub fn put_in_front_of(origin: OriginFor<T>, lighter: T::AccountId) -> DispatchResult {
+			let heavier = ensure_signed(origin)?;
 			List::<T>::put_in_front_of(
-				&heavier,
 				&lighter,
+				&heavier,
 				Box::new(T::VoteWeightProvider::vote_weight),
 			)
 			.map_err(Into::into)
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index ccbad2fa8c6e0..543a1abb8d847 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -210,7 +210,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&11, 16);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 10, 11));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
 			// then
 			assert_eq!(
@@ -233,7 +233,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&10, 15);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 10, 11));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
 			// then
 			assert_eq!(
@@ -253,7 +253,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&3, 999);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(4), 3));
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
@@ -275,7 +275,7 @@ mod pallet {
 				StakingMock::set_vote_weight_of(&3, 1_000);
 
 				// when
-				assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 5, 3));
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(3), 5));
 
 				// then
 				assert_eq!(
@@ -295,7 +295,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&2, 999);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 3));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(3), 2));
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
@@ -312,7 +312,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&3, 999);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(4), 3));
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3])]);
@@ -329,7 +329,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&2, 999);
 
 			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 5));
+			assert_ok!(BagsList::put_in_front_of(Origin::signed(5), 2));
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![5, 2, 3, 4])]);
@@ -346,7 +346,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&4, 999);
 
 			// when
-			BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();
+			BagsList::put_in_front_of(Origin::signed(2), 4).unwrap();
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]);
@@ -363,7 +363,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&3, 1_000);
 
 			// when
-			BagsList::put_in_front_of(Origin::signed(0), 5, 3).unwrap();
+			BagsList::put_in_front_of(Origin::signed(3), 5).unwrap();
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
@@ -380,7 +380,7 @@ mod pallet {
 			StakingMock::set_vote_weight_of(&4, 999);
 
 			// when
-			BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();
+			BagsList::put_in_front_of(Origin::signed(2), 4).unwrap();
 
 			// then
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
@@ -398,7 +398,7 @@ mod pallet {
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 2, 3),
+				BagsList::put_in_front_of(Origin::signed(3), 2),
 				crate::pallet::Error::<Runtime>::NotHeavier
 			);
 		});
@@ -424,7 +424,7 @@ mod pallet {
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 5, 6),
+				BagsList::put_in_front_of(Origin::signed(6), 5),
 				crate::pallet::Error::<Runtime>::NotHeavier
 			);
 		});
@@ -441,7 +441,7 @@ mod pallet {
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 4, 5),
+				BagsList::put_in_front_of(Origin::signed(5), 4),
 				crate::pallet::Error::<Runtime>::IdNotFound
 			);
 		});
@@ -455,7 +455,7 @@ mod pallet {
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 5, 4),
+				BagsList::put_in_front_of(Origin::signed(4), 5),
 				crate::pallet::Error::<Runtime>::IdNotFound
 			);
 		});
@@ -469,7 +469,7 @@ mod pallet {
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(0), 1, 4),
+				BagsList::put_in_front_of(Origin::signed(4), 1),
 				crate::pallet::Error::<Runtime>::NotInSameBag
 			);
 		});

From b75457f6844d096281f5cbd4e711adac17254fd2 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 23 Sep 2021 12:38:05 -0700
Subject: [PATCH 14/30] Add test cases for insert_at_unchecked

---
 frame/bags-list/src/list/mod.rs   |  6 +-
 frame/bags-list/src/list/tests.rs | 96 +++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index e9e8455c2385b..67ac8712a2697 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -422,8 +422,10 @@ impl<T: Config> List<T> {
 
 	/// Insert `node` directly in front of `at`.
 	///
-	/// WARNING: we do not check that `node` or `at` are valid. This will panic if `at.bag_upper` is
-	/// not a bag that already exists in storage.
+	/// WARNINGS:
+	/// - we do not check that `node` or `at` are valid.
+	/// - this will panic if `at.bag_upper` is not a bag that already exists in storage.
+	/// - this does not update [`crate::CounterForListNodes`].
 	fn insert_at_unchecked(mut at: Node<T>, mut node: Node<T>) {
 		// connect `heavier_node` to its new `prev`.
 		node.prev = at.prev.clone();
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index b3779d5e1a7ef..e108a2beceef9 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -391,6 +391,102 @@ mod list {
 			));
 		});
 	}
+
+	#[test]
+	fn insert_at_unchecked_at_is_only_node() {
+		// Note that `insert_at_unchecked` tests should fail post checks because it does not
+		// increment the node counter.
+		ExtBuilder::default().build_and_execute_no_post_check(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			// implicitly also test that `node`'s `prev`/`next` are correctly re-assigned.
+			let node_42 =
+				Node::<Runtime> { id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000 };
+			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
+
+			let node_1 = crate::ListNodes::<Runtime>::get(&1).unwrap();
+
+			// when
+			List::<Runtime>::insert_at_unchecked(node_1, node_42);
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![42, 1]), (1_000, vec![2, 3, 4])]
+			);
+		})
+	}
+
+	#[test]
+	fn insert_at_unchecked_at_is_head() {
+		ExtBuilder::default().build_and_execute_no_post_check(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			// implicitly also test that `node`'s `prev`/`next` are correctly re-assigned.
+			let node_42 = Node::<Runtime> { id: 42, prev: Some(4), next: None, bag_upper: 1_000 };
+			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
+
+			let node_2 = crate::ListNodes::<Runtime>::get(&2).unwrap();
+
+			// when
+			List::<Runtime>::insert_at_unchecked(node_2, node_42);
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![42, 2, 3, 4])]
+			);
+		})
+	}
+
+	#[test]
+	fn insert_at_unchecked_at_is_non_terminal() {
+		ExtBuilder::default().build_and_execute_no_post_check(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			// implicitly also test that `node`'s `prev`/`next` are correctly re-assigned.
+			let node_42 = Node::<Runtime> { id: 42, prev: None, next: Some(2), bag_upper: 1_000 };
+			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
+
+			let node_3 = crate::ListNodes::<Runtime>::get(&3).unwrap();
+
+			// when
+			List::<Runtime>::insert_at_unchecked(node_3, node_42);
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![2, 42, 3, 4])]
+			);
+		})
+	}
+
+	#[test]
+	fn insert_at_unchecked_at_is_tail() {
+		ExtBuilder::default().build_and_execute_no_post_check(|| {
+			// given
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+
+			// implicitly also test that `node`'s `prev`/`next` are correctly re-assigned.
+			let node_42 =
+				Node::<Runtime> { id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000 };
+			assert!(!crate::ListNodes::<Runtime>::contains_key(42));
+
+			let node_4 = crate::ListNodes::<Runtime>::get(&4).unwrap();
+
+			// when
+			List::<Runtime>::insert_at_unchecked(node_4, node_42);
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![(10, vec![1]), (1_000, vec![2, 3, 42, 4])]
+			);
+		})
+	}
 }
 
 mod bags {

From fd1daeb0b0d6db4bc0f275f7ceea8ea308e6227d Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Fri, 24 Sep 2021 19:54:31 -0700
Subject: [PATCH 15/30] Move counter update to insert_at; fix comments

---
 frame/bags-list/src/list/mod.rs | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 67ac8712a2697..0b7ec48813c7e 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -409,25 +409,21 @@ impl<T: Config> List<T> {
 		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
 		// was removed.
 		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
+		// insert `heavier_node` directly in front of `lighter_node`. This will update both nodes
+		// in storage and update the node counter.
 		Self::insert_at_unchecked(lighter_node, heavier_node);
 
-		// Increment the counter, which got decremented by `Self::remove`, since we just re-inserted
-		// `heavier_node`.
-		crate::CounterForListNodes::<T>::mutate(|prev_count| {
-			*prev_count = prev_count.saturating_add(1)
-		});
-
 		Ok(())
 	}
 
 	/// Insert `node` directly in front of `at`.
 	///
 	/// WARNINGS:
-	/// - we do not check that `node` or `at` are valid.
+	/// - this is a naive function in that it does not check if `node` belongs to the same bag as
+	/// `at`. It is expected that the call site will check preconditions.
 	/// - this will panic if `at.bag_upper` is not a bag that already exists in storage.
-	/// - this does not update [`crate::CounterForListNodes`].
 	fn insert_at_unchecked(mut at: Node<T>, mut node: Node<T>) {
-		// connect `heavier_node` to its new `prev`.
+		// connect `node` to its new `prev`.
 		node.prev = at.prev.clone();
 		if let Some(mut prev) = at.prev() {
 			prev.next = Some(node.id().clone());
@@ -460,6 +456,11 @@ impl<T: Config> List<T> {
 		// write the updated nodes to storage.
 		at.put();
 		node.put();
+
+		// account for `node` being added to the list.
+		crate::CounterForListNodes::<T>::mutate(|prev_count| {
+			*prev_count = prev_count.saturating_add(1)
+		});
 	}
 
 	/// Sanity check the list.

From f3f992c99a17fea96be3a0c67c49cc75a82fb081 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Mon, 8 Nov 2021 20:18:46 +0100
Subject: [PATCH 16/30] Address some feedback

---
 frame/bags-list/src/lib.rs        |  4 ----
 frame/bags-list/src/list/mod.rs   |  6 +++---
 frame/bags-list/src/list/tests.rs | 20 +++++++++-----------
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 12d5bba34831c..5adba5f5baf8c 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -185,10 +185,6 @@ pub mod pallet {
 		IdNotFound,
 		/// An Id does not have a greater vote weight than another Id.
 		NotHeavier,
-		/// Heavier weight Id is already in a higher position than a lesser weight Id.
-		AlreadyHigherPosition,
-		/// Bag could not be found. This is a system logic error that should never happen.
-		BagNotFound,
 	}
 
 	#[pallet::call]
diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 153c56b20c2e0..fae3b68a52e8d 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -384,8 +384,8 @@ impl<T: Config> List<T> {
 		})
 	}
 
-	/// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in
-	/// same bag  and the `weight_of` `lighter_id` must be less than that of `heavier_id`.
+	/// Put `heavier_id` to the position directly in front of `lighter_id`. Both ids must be in the
+	/// same bag and the `weight_of` `lighter_id` must be less than that of `heavier_id`.
 	pub(crate) fn put_in_front_of(
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
@@ -440,7 +440,7 @@ impl<T: Config> List<T> {
 			// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
 			// tail.
 			let mut bag =
-				Bag::<T>::get(at.bag_upper).expect("given nodes must always have a valid. qed.");
+				Bag::<T>::get(at.bag_upper).expect("given nodes must always have a valid bag. qed.");
 
 			if node.prev == None {
 				bag.head = Some(node.id().clone())
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index e108a2beceef9..ed6ff40668a27 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -368,7 +368,7 @@ mod list {
 	}
 
 	#[test]
-	#[should_panic = "given nodes must always have a valid. qed."]
+	#[should_panic = "given nodes must always have a valid bag. qed."]
 	fn put_in_front_of_panics_if_bag_not_found() {
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			let node_10_no_bag = Node::<Runtime> { id: 10, prev: None, next: None, bag_upper: 15 };
@@ -384,18 +384,16 @@ mod list {
 
 			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
 
-			// then
-			assert_storage_noop!(assert_eq!(
-				List::<Runtime>::put_in_front_of(&10, &11, weight_fn).unwrap_err(),
-				crate::pallet::Error::<Runtime>::BagNotFound
-			));
+			// then .. this panics
+			let _ = List::<Runtime>::put_in_front_of(&10, &11, weight_fn);
 		});
 	}
 
 	#[test]
 	fn insert_at_unchecked_at_is_only_node() {
-		// Note that `insert_at_unchecked` tests should fail post checks because it does not
-		// increment the node counter.
+		// Note that this `insert_at_unchecked` test should fail post checks because node 42 does not
+		// get re-assigned the correct bagu pper. This is because `insert_at_unchecked` assumes
+		// both nodes are already in the same bag with the correct bag upper.
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
@@ -420,7 +418,7 @@ mod list {
 
 	#[test]
 	fn insert_at_unchecked_at_is_head() {
-		ExtBuilder::default().build_and_execute_no_post_check(|| {
+		ExtBuilder::default().build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
@@ -443,7 +441,7 @@ mod list {
 
 	#[test]
 	fn insert_at_unchecked_at_is_non_terminal() {
-		ExtBuilder::default().build_and_execute_no_post_check(|| {
+		ExtBuilder::default().build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
@@ -466,7 +464,7 @@ mod list {
 
 	#[test]
 	fn insert_at_unchecked_at_is_tail() {
-		ExtBuilder::default().build_and_execute_no_post_check(|| {
+		ExtBuilder::default().build_and_execute(|| {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 

From 927a3836e28c4f05de8eb29a1caed13cec285ee3 Mon Sep 17 00:00:00 2001
From: Zeke Mostov <z.mostov@gmail.com>
Date: Wed, 10 Nov 2021 10:35:05 +0100
Subject: [PATCH 17/30] Make voteweight constructed with parameter_types

---
 frame/bags-list/src/list/tests.rs | 64 ++++++++++++++++++-------------
 frame/bags-list/src/mock.rs       | 25 ++----------
 2 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index ed6ff40668a27..e61ff8f08a8d6 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -109,34 +109,46 @@ fn remove_last_node_in_bags_cleans_bag() {
 
 #[test]
 fn migrate_works() {
-	ExtBuilder::default().add_aux_data().build_and_execute(|| {
-		// given
-		assert_eq!(
-			List::<Runtime>::get_bags(),
-			vec![(10, vec![1]), (20, vec![710, 711]), (1_000, vec![2, 3, 4]), (2_000, vec![712])]
-		);
-		let old_thresholds = <Runtime as Config>::BagThresholds::get();
-		assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]);
+	ExtBuilder::default()
+		.add_ids(vec![(710, 15), (711, 16), (712, 2_000)])
+		.build_and_execute(|| {
+			StakingMock::set_vote_weight_of(&710, 15);
+			StakingMock::set_vote_weight_of(&711, 16);
+			StakingMock::set_vote_weight_of(&712, 2_000);
 
-		// when the new thresholds adds `15` and removes `2_000`
-		const NEW_THRESHOLDS: &'static [VoteWeight] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000];
-		BagThresholds::set(NEW_THRESHOLDS);
-		// and we call
-		List::<Runtime>::migrate(old_thresholds);
+			// given
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![
+					(10, vec![1]),
+					(20, vec![710, 711]),
+					(1_000, vec![2, 3, 4]),
+					(2_000, vec![712])
+				]
+			);
+			let old_thresholds = <Runtime as Config>::BagThresholds::get();
+			assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]);
 
-		// then
-		assert_eq!(
-			List::<Runtime>::get_bags(),
-			vec![
-				(10, vec![1]),
-				(15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15
-				(20, vec![711]),
-				(1_000, vec![2, 3, 4]),
-				// nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000
-				(10_000, vec![712]),
-			]
-		);
-	});
+			// when the new thresholds adds `15` and removes `2_000`
+			const NEW_THRESHOLDS: &'static [VoteWeight] =
+				&[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000];
+			BagThresholds::set(NEW_THRESHOLDS);
+			// and we call
+			List::<Runtime>::migrate(old_thresholds);
+
+			// then
+			assert_eq!(
+				List::<Runtime>::get_bags(),
+				vec![
+					(10, vec![1]),
+					(15, vec![710]), // nodes in range 11 ..= 15 move from bag 20 to bag 15
+					(20, vec![711]),
+					(1_000, vec![2, 3, 4]),
+					// nodes in range 1_001 ..= 2_000 move from bag 2_000 to bag 10_000
+					(10_000, vec![712]),
+				]
+			);
+		});
 }
 
 mod list {
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index 374318c491d7f..e303c59614ea1 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -21,30 +21,21 @@ use super::*;
 use crate::{self as bags_list};
 use frame_election_provider_support::VoteWeight;
 use frame_support::parameter_types;
-use std::{cell::RefCell, collections::HashMap};
+use std::collections::HashMap;
 
 pub type AccountId = u32;
 pub type Balance = u32;
 
-thread_local! {
-	static NEXT_VOTE_WEIGHT_MAP: RefCell<HashMap<AccountId, VoteWeight>>
-		= RefCell::new(Default::default());
-}
-
 parameter_types! {
 	// Set the vote weight for any id who's weight has _not_ been set with `set_vote_weight_of`.
 	pub static NextVoteWeight: VoteWeight = 0;
+	pub static NextVoteWeightMap: HashMap<AccountId, VoteWeight> = Default::default();
 }
 
 pub struct StakingMock;
 impl frame_election_provider_support::VoteWeightProvider<AccountId> for StakingMock {
 	fn vote_weight(id: &AccountId) -> VoteWeight {
-		NEXT_VOTE_WEIGHT_MAP.with(|m| {
-			m.borrow_mut()
-				.get(id)
-				.map(|weight| weight.clone())
-				.unwrap_or(NextVoteWeight::get())
-		})
+		*NextVoteWeightMap::get().get(id).unwrap_or(&NextVoteWeight::get())
 	}
 
 	#[cfg(any(feature = "runtime-benchmarks", test))]
@@ -120,16 +111,6 @@ impl ExtBuilder {
 		self
 	}
 
-	/// Set vote weights of some Id's used for special case
-	pub(crate) fn add_aux_data(mut self) -> Self {
-		StakingMock::set_vote_weight_of(&710, 15);
-		StakingMock::set_vote_weight_of(&711, 16);
-		StakingMock::set_vote_weight_of(&712, 2_000);
-		self.ids.extend([(710, 15), (711, 16), (712, 2_000)].iter());
-
-		self
-	}
-
 	pub(crate) fn build(self) -> sp_io::TestExternalities {
 		sp_tracing::try_init_simple();
 		let storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();

From 387ab0dad45da9500de0b6f8efb2323718f1d777 Mon Sep 17 00:00:00 2001
From: Zeke Mostov <z.mostov@gmail.com>
Date: Wed, 10 Nov 2021 11:04:51 +0100
Subject: [PATCH 18/30] Always set vote weight for Ids in build

---
 frame/bags-list/src/list/tests.rs |  4 ----
 frame/bags-list/src/mock.rs       |  1 +
 frame/bags-list/src/tests.rs      | 37 +++++++++----------------------
 3 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index e61ff8f08a8d6..9b8e6f8498324 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -112,10 +112,6 @@ fn migrate_works() {
 	ExtBuilder::default()
 		.add_ids(vec![(710, 15), (711, 16), (712, 2_000)])
 		.build_and_execute(|| {
-			StakingMock::set_vote_weight_of(&710, 15);
-			StakingMock::set_vote_weight_of(&711, 16);
-			StakingMock::set_vote_weight_of(&712, 2_000);
-
 			// given
 			assert_eq!(
 				List::<Runtime>::get_bags(),
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index e303c59614ea1..eb4e4b01f60e0 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -119,6 +119,7 @@ impl ExtBuilder {
 		ext.execute_with(|| {
 			for (id, weight) in GENESIS_IDS.iter().chain(self.ids.iter()) {
 				frame_support::assert_ok!(List::<Runtime>::insert(*id, *weight));
+				StakingMock::set_vote_weight_of(id, *weight);
 			}
 		});
 
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index 543a1abb8d847..084d125847b87 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -35,7 +35,7 @@ mod pallet {
 			);
 
 			// when increasing vote weight to the level of non-existent bag
-			NextVoteWeight::set(2_000);
+			StakingMock::set_vote_weight_of(&42, 2_000);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 42));
 
 			// then a new bag is created and the id moves into it
@@ -45,7 +45,7 @@ mod pallet {
 			);
 
 			// when decreasing weight within the range of the current bag
-			NextVoteWeight::set(1001);
+			StakingMock::set_vote_weight_of(&42, 1_001);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 42));
 
 			// then the id does not move
@@ -55,7 +55,7 @@ mod pallet {
 			);
 
 			// when reducing weight to the level of a non-existent bag
-			NextVoteWeight::set(30);
+			StakingMock::set_vote_weight_of(&42, 30);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 42));
 
 			// then a new bag is created and the id moves into it
@@ -65,7 +65,7 @@ mod pallet {
 			);
 
 			// when increasing weight to the level of a pre-existing bag
-			NextVoteWeight::set(500);
+			StakingMock::set_vote_weight_of(&42, 500);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 42));
 
 			// then the id moves into that bag
@@ -85,7 +85,7 @@ mod pallet {
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
 			// when
-			NextVoteWeight::set(10);
+			StakingMock::set_vote_weight_of(&4, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 4));
 
 			// then
@@ -93,6 +93,7 @@ mod pallet {
 			assert_eq!(Bag::<Runtime>::get(1_000).unwrap(), Bag::new(Some(2), Some(3), 1_000));
 
 			// when
+			StakingMock::set_vote_weight_of(&3, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 3));
 
 			// then
@@ -103,6 +104,7 @@ mod pallet {
 			assert_eq!(get_list_as_ids(), vec![2u32, 1, 4, 3]);
 
 			// when
+			StakingMock::set_vote_weight_of(&2, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 2));
 
 			// then
@@ -117,7 +119,7 @@ mod pallet {
 	fn rebag_head_works() {
 		ExtBuilder::default().build_and_execute(|| {
 			// when
-			NextVoteWeight::set(10);
+			StakingMock::set_vote_weight_of(&2, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 2));
 
 			// then
@@ -125,6 +127,7 @@ mod pallet {
 			assert_eq!(Bag::<Runtime>::get(1_000).unwrap(), Bag::new(Some(3), Some(4), 1_000));
 
 			// when
+			StakingMock::set_vote_weight_of(&3, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 3));
 
 			// then
@@ -132,6 +135,7 @@ mod pallet {
 			assert_eq!(Bag::<Runtime>::get(1_000).unwrap(), Bag::new(Some(4), Some(4), 1_000));
 
 			// when
+			StakingMock::set_vote_weight_of(&4, 10);
 			assert_ok!(BagsList::rebag(Origin::signed(0), 4));
 
 			// then
@@ -206,9 +210,6 @@ mod pallet {
 				vec![(10, vec![1]), (20, vec![10, 11]), (1_000, vec![2, 3, 4])]
 			);
 
-			StakingMock::set_vote_weight_of(&10, 15);
-			StakingMock::set_vote_weight_of(&11, 16);
-
 			// when
 			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
@@ -229,9 +230,6 @@ mod pallet {
 				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
 			);
 
-			StakingMock::set_vote_weight_of(&11, 16);
-			StakingMock::set_vote_weight_of(&10, 15);
-
 			// when
 			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
@@ -249,7 +247,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
 
-			StakingMock::set_vote_weight_of(&4, 1_000);
 			StakingMock::set_vote_weight_of(&3, 999);
 
 			// when
@@ -272,7 +269,6 @@ mod pallet {
 				);
 
 				StakingMock::set_vote_weight_of(&5, 999);
-				StakingMock::set_vote_weight_of(&3, 1_000);
 
 				// when
 				assert_ok!(BagsList::put_in_front_of(Origin::signed(3), 5));
@@ -291,7 +287,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
-			StakingMock::set_vote_weight_of(&3, 1_000);
 			StakingMock::set_vote_weight_of(&2, 999);
 
 			// when
@@ -308,7 +303,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
-			StakingMock::set_vote_weight_of(&4, 1_000);
 			StakingMock::set_vote_weight_of(&3, 999);
 
 			// when
@@ -325,7 +319,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
 
-			StakingMock::set_vote_weight_of(&5, 1_000);
 			StakingMock::set_vote_weight_of(&2, 999);
 
 			// when
@@ -342,7 +335,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
 
-			StakingMock::set_vote_weight_of(&2, 1_000);
 			StakingMock::set_vote_weight_of(&4, 999);
 
 			// when
@@ -359,9 +351,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);
 
-			StakingMock::set_vote_weight_of(&5, 900);
-			StakingMock::set_vote_weight_of(&3, 1_000);
-
 			// when
 			BagsList::put_in_front_of(Origin::signed(3), 5).unwrap();
 
@@ -376,7 +365,6 @@ mod pallet {
 			// given
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
-			StakingMock::set_vote_weight_of(&2, 1_000);
 			StakingMock::set_vote_weight_of(&4, 999);
 
 			// when
@@ -394,7 +382,6 @@ mod pallet {
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
 			StakingMock::set_vote_weight_of(&3, 999);
-			StakingMock::set_vote_weight_of(&2, 1_000);
 
 			// then
 			assert_noop!(
@@ -406,8 +393,6 @@ mod pallet {
 
 	#[test]
 	fn put_in_front_of_errors_if_heavier_is_equal_weight_to_lighter() {
-		use mock::StakingMock;
-
 		ExtBuilder::default().add_ids(vec![(5, 15), (6, 15)]).build_and_execute(|| {
 			//	note insertion order ^^^^^^
 
@@ -417,8 +402,6 @@ mod pallet {
 				vec![(10, vec![1]), (20, vec![5, 6]), (1_000, vec![2, 3, 4])]
 			);
 
-			StakingMock::set_vote_weight_of(&5, 15);
-			StakingMock::set_vote_weight_of(&6, 15);
 			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&5), 15);
 			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&6), 15);
 

From 7d5a1ca3ecd92be266b8c89060950fbb041c38da Mon Sep 17 00:00:00 2001
From: Zeke Mostov <z.mostov@gmail.com>
Date: Wed, 10 Nov 2021 11:31:50 +0100
Subject: [PATCH 19/30] Add skip_genesis_ids

---
 frame/bags-list/src/list/tests.rs |  4 +-
 frame/bags-list/src/mock.rs       | 14 ++++++-
 frame/bags-list/src/tests.rs      | 64 ++++++++++++-------------------
 3 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index 9b8e6f8498324..a3b974b25540a 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -378,7 +378,7 @@ mod list {
 	#[test]
 	#[should_panic = "given nodes must always have a valid bag. qed."]
 	fn put_in_front_of_panics_if_bag_not_found() {
-		ExtBuilder::default().build_and_execute_no_post_check(|| {
+		ExtBuilder::default().skip_genesis_ids().build_and_execute_no_post_check(|| {
 			let node_10_no_bag = Node::<Runtime> { id: 10, prev: None, next: None, bag_upper: 15 };
 			let node_11_no_bag = Node::<Runtime> { id: 11, prev: None, next: None, bag_upper: 15 };
 
@@ -388,7 +388,7 @@ mod list {
 			StakingMock::set_vote_weight_of(&10, 14);
 			StakingMock::set_vote_weight_of(&11, 15);
 			assert!(!ListBags::<Runtime>::contains_key(15));
-			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
+			assert_eq!(List::<Runtime>::get_bags(), vec![]);
 
 			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
 
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index eb4e4b01f60e0..ece0c7cea6a6d 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -101,9 +101,15 @@ pub(crate) const GENESIS_IDS: [(AccountId, VoteWeight); 4] =
 #[derive(Default)]
 pub struct ExtBuilder {
 	ids: Vec<(AccountId, VoteWeight)>,
+	skip_genesis_ids: bool,
 }
 
 impl ExtBuilder {
+	pub(crate) fn skip_genesis_ids(mut self) -> Self {
+		self.skip_genesis_ids = true;
+		self
+	}
+
 	/// Add some AccountIds to insert into `List`.
 	#[cfg(test)]
 	pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, VoteWeight)>) -> Self {
@@ -115,9 +121,15 @@ impl ExtBuilder {
 		sp_tracing::try_init_simple();
 		let storage = frame_system::GenesisConfig::default().build_storage::<Runtime>().unwrap();
 
+		let ids_with_weight: Vec<_> = if self.skip_genesis_ids {
+			self.ids.iter().collect()
+		} else {
+			GENESIS_IDS.iter().chain(self.ids.iter()).collect()
+		};
+
 		let mut ext = sp_io::TestExternalities::from(storage);
 		ext.execute_with(|| {
-			for (id, weight) in GENESIS_IDS.iter().chain(self.ids.iter()) {
+			for (id, weight) in ids_with_weight {
 				frame_support::assert_ok!(List::<Runtime>::insert(*id, *weight));
 				StakingMock::set_vote_weight_of(id, *weight);
 			}
diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs
index 084d125847b87..836096c5aae74 100644
--- a/frame/bags-list/src/tests.rs
+++ b/frame/bags-list/src/tests.rs
@@ -203,42 +203,36 @@ mod pallet {
 
 	#[test]
 	fn put_in_front_of_two_node_bag_heavier_is_tail() {
-		ExtBuilder::default().add_ids(vec![(10, 15), (11, 16)]).build_and_execute(|| {
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![10, 11]), (1_000, vec![2, 3, 4])]
-			);
+		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])]);
 
-			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
-			// then
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
-			);
-		});
+				// then
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![11, 10])]);
+			});
 	}
 
 	#[test]
 	fn put_in_front_of_two_node_bag_heavier_is_head() {
-		ExtBuilder::default().add_ids(vec![(11, 16), (10, 15)]).build_and_execute(|| {
-			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
-			);
+		ExtBuilder::default()
+			.skip_genesis_ids()
+			.add_ids(vec![(11, 16), (10, 15)])
+			.build_and_execute(|| {
+				// given
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![11, 10])]);
 
-			// when
-			assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
+				// when
+				assert_ok!(BagsList::put_in_front_of(Origin::signed(11), 10));
 
-			// then
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![11, 10]), (1_000, vec![2, 3, 4])]
-			);
-		});
+				// then
+				assert_eq!(List::<Runtime>::get_bags(), vec![(20, vec![11, 10])]);
+			});
 	}
 
 	#[test]
@@ -393,21 +387,13 @@ mod pallet {
 
 	#[test]
 	fn put_in_front_of_errors_if_heavier_is_equal_weight_to_lighter() {
-		ExtBuilder::default().add_ids(vec![(5, 15), (6, 15)]).build_and_execute(|| {
-			//	note insertion order ^^^^^^
-
+		ExtBuilder::default().build_and_execute(|| {
 			// given
-			assert_eq!(
-				List::<Runtime>::get_bags(),
-				vec![(10, vec![1]), (20, vec![5, 6]), (1_000, vec![2, 3, 4])]
-			);
-
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&5), 15);
-			assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&6), 15);
+			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
 			// then
 			assert_noop!(
-				BagsList::put_in_front_of(Origin::signed(6), 5),
+				BagsList::put_in_front_of(Origin::signed(3), 4),
 				crate::pallet::Error::<Runtime>::NotHeavier
 			);
 		});

From 2add92716e17a47980be9d6d73ae67155bfec807 Mon Sep 17 00:00:00 2001
From: Zeke Mostov <z.mostov@gmail.com>
Date: Wed, 10 Nov 2021 11:49:12 +0100
Subject: [PATCH 20/30] Do not pass weight fn to List put_in_front_of

---
 frame/bags-list/src/lib.rs        |  7 +------
 frame/bags-list/src/list/mod.rs   | 27 +++++++++++++++------------
 frame/bags-list/src/list/tests.rs |  4 +---
 frame/bags-list/src/mock.rs       |  2 ++
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 5adba5f5baf8c..17a3dee3c62f0 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -216,12 +216,7 @@ pub mod pallet {
 		#[pallet::weight(T::WeightInfo::put_in_front_of())]
 		pub fn put_in_front_of(origin: OriginFor<T>, lighter: T::AccountId) -> DispatchResult {
 			let heavier = ensure_signed(origin)?;
-			List::<T>::put_in_front_of(
-				&lighter,
-				&heavier,
-				Box::new(T::VoteWeightProvider::vote_weight),
-			)
-			.map_err(Into::into)
+			List::<T>::put_in_front_of(&lighter, &heavier).map_err(Into::into)
 		}
 	}
 
diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index fae3b68a52e8d..b5856a6b1dc8d 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -130,7 +130,7 @@ impl<T: Config> List<T> {
 	pub fn migrate(old_thresholds: &[VoteWeight]) -> u32 {
 		let new_thresholds = T::BagThresholds::get();
 		if new_thresholds == old_thresholds {
-			return 0
+			return 0;
 		}
 
 		// we can't check all preconditions, but we can check one
@@ -163,7 +163,7 @@ impl<T: Config> List<T> {
 			if !affected_old_bags.insert(affected_bag) {
 				// If the previous threshold list was [10, 20], and we insert [3, 5], then there's
 				// no point iterating through bag 10 twice.
-				continue
+				continue;
 			}
 
 			if let Some(bag) = Bag::<T>::get(affected_bag) {
@@ -175,7 +175,7 @@ impl<T: Config> List<T> {
 		// a removed bag means that all members of that bag must be rebagged
 		for removed_bag in removed_bags.clone() {
 			if !affected_old_bags.insert(removed_bag) {
-				continue
+				continue;
 			}
 
 			if let Some(bag) = Bag::<T>::get(removed_bag) {
@@ -263,7 +263,7 @@ impl<T: Config> List<T> {
 	/// Returns an error if the list already contains `id`.
 	pub(crate) fn insert(id: T::AccountId, weight: VoteWeight) -> Result<(), Error> {
 		if Self::contains(&id) {
-			return Err(Error::Duplicate)
+			return Err(Error::Duplicate);
 		}
 
 		let bag_weight = notional_bag_for::<T>(weight);
@@ -389,7 +389,6 @@ impl<T: Config> List<T> {
 	pub(crate) fn put_in_front_of(
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
-		weight_of: Box<dyn Fn(&T::AccountId) -> VoteWeight>,
 	) -> Result<(), crate::pallet::Error<T>> {
 		use crate::pallet;
 		use frame_support::ensure;
@@ -400,7 +399,11 @@ impl<T: Config> List<T> {
 		ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);
 
 		// this is the most expensive check, so we do it last.
-		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
+		ensure!(
+			T::VoteWeightProvider::vote_weight(&heavier_id)
+				> T::VoteWeightProvider::vote_weight(&lighter_id),
+			pallet::Error::NotHeavier
+		);
 
 		// remove the heavier node from this list. Note that this removes the node from storage and
 		// decrements the node counter.
@@ -439,8 +442,8 @@ impl<T: Config> List<T> {
 			// since `node` is always in front of `at` we know that 1) there is always at least 2
 			// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
 			// tail.
-			let mut bag =
-				Bag::<T>::get(at.bag_upper).expect("given nodes must always have a valid bag. qed.");
+			let mut bag = Bag::<T>::get(at.bag_upper)
+				.expect("given nodes must always have a valid bag. qed.");
 
 			if node.prev == None {
 				bag.head = Some(node.id().clone())
@@ -646,7 +649,7 @@ impl<T: Config> Bag<T> {
 				// infinite loop.
 				debug_assert!(false, "system logic error: inserting a node who has the id of tail");
 				crate::log!(warn, "system logic error: inserting a node who has the id of tail");
-				return
+				return;
 			};
 		}
 
@@ -852,9 +855,9 @@ impl<T: Config> Node<T> {
 			"node does not exist in the expected bag"
 		);
 
-		let non_terminal_check = !self.is_terminal() &&
-			expected_bag.head.as_ref() != Some(id) &&
-			expected_bag.tail.as_ref() != Some(id);
+		let non_terminal_check = !self.is_terminal()
+			&& expected_bag.head.as_ref() != Some(id)
+			&& expected_bag.tail.as_ref() != Some(id);
 		let terminal_check =
 			expected_bag.head.as_ref() == Some(id) || expected_bag.tail.as_ref() == Some(id);
 		frame_support::ensure!(
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index a3b974b25540a..ba62b9423c1ce 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -390,10 +390,8 @@ mod list {
 			assert!(!ListBags::<Runtime>::contains_key(15));
 			assert_eq!(List::<Runtime>::get_bags(), vec![]);
 
-			let weight_fn = Box::new(<Runtime as Config>::VoteWeightProvider::vote_weight);
-
 			// then .. this panics
-			let _ = List::<Runtime>::put_in_front_of(&10, &11, weight_fn);
+			let _ = List::<Runtime>::put_in_front_of(&10, &11);
 		});
 	}
 
diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs
index ece0c7cea6a6d..6545e563a3efe 100644
--- a/frame/bags-list/src/mock.rs
+++ b/frame/bags-list/src/mock.rs
@@ -105,6 +105,8 @@ pub struct ExtBuilder {
 }
 
 impl ExtBuilder {
+	/// Skip adding the default genesis ids to the list.
+	#[cfg(test)]
 	pub(crate) fn skip_genesis_ids(mut self) -> Self {
 		self.skip_genesis_ids = true;
 		self

From d135b8f647b76eb1729916678528c7c6a3942729 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 17 Nov 2021 14:11:31 +0100
Subject: [PATCH 21/30] Remove remants of CounterForListNodes

---
 frame/bags-list/src/list/mod.rs | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index e4f7dea7d2818..42b3be26e670f 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -452,11 +452,6 @@ impl<T: Config> List<T> {
 		// write the updated nodes to storage.
 		at.put();
 		node.put();
-
-		// account for `node` being added to the list.
-		crate::CounterForListNodes::<T>::mutate(|prev_count| {
-			*prev_count = prev_count.saturating_add(1)
-		});
 	}
 
 	/// Sanity check the list.

From 7530d1a90bbd78b90a3b043f5efaf35b7a4aaa22 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 17 Nov 2021 14:24:34 +0100
Subject: [PATCH 22/30] fmt

---
 frame/bags-list/src/list/mod.rs   | 20 ++++++++++----------
 frame/bags-list/src/list/tests.rs |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 42b3be26e670f..25b339e842ea8 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -131,7 +131,7 @@ impl<T: Config> List<T> {
 	pub fn migrate(old_thresholds: &[VoteWeight]) -> u32 {
 		let new_thresholds = T::BagThresholds::get();
 		if new_thresholds == old_thresholds {
-			return 0;
+			return 0
 		}
 
 		// we can't check all preconditions, but we can check one
@@ -164,7 +164,7 @@ impl<T: Config> List<T> {
 			if !affected_old_bags.insert(affected_bag) {
 				// If the previous threshold list was [10, 20], and we insert [3, 5], then there's
 				// no point iterating through bag 10 twice.
-				continue;
+				continue
 			}
 
 			if let Some(bag) = Bag::<T>::get(affected_bag) {
@@ -176,7 +176,7 @@ impl<T: Config> List<T> {
 		// a removed bag means that all members of that bag must be rebagged
 		for removed_bag in removed_bags.clone() {
 			if !affected_old_bags.insert(removed_bag) {
-				continue;
+				continue
 			}
 
 			if let Some(bag) = Bag::<T>::get(removed_bag) {
@@ -264,7 +264,7 @@ impl<T: Config> List<T> {
 	/// Returns an error if the list already contains `id`.
 	pub(crate) fn insert(id: T::AccountId, weight: VoteWeight) -> Result<(), Error> {
 		if Self::contains(&id) {
-			return Err(Error::Duplicate);
+			return Err(Error::Duplicate)
 		}
 
 		let bag_weight = notional_bag_for::<T>(weight);
@@ -393,8 +393,8 @@ impl<T: Config> List<T> {
 
 		// this is the most expensive check, so we do it last.
 		ensure!(
-			T::VoteWeightProvider::vote_weight(&heavier_id)
-				> T::VoteWeightProvider::vote_weight(&lighter_id),
+			T::VoteWeightProvider::vote_weight(&heavier_id) >
+				T::VoteWeightProvider::vote_weight(&lighter_id),
 			pallet::Error::NotHeavier
 		);
 
@@ -637,7 +637,7 @@ impl<T: Config> Bag<T> {
 				// infinite loop.
 				debug_assert!(false, "system logic error: inserting a node who has the id of tail");
 				crate::log!(warn, "system logic error: inserting a node who has the id of tail");
-				return;
+				return
 			};
 		}
 
@@ -843,9 +843,9 @@ impl<T: Config> Node<T> {
 			"node does not exist in the expected bag"
 		);
 
-		let non_terminal_check = !self.is_terminal()
-			&& expected_bag.head.as_ref() != Some(id)
-			&& expected_bag.tail.as_ref() != Some(id);
+		let non_terminal_check = !self.is_terminal() &&
+			expected_bag.head.as_ref() != Some(id) &&
+			expected_bag.tail.as_ref() != Some(id);
 		let terminal_check =
 			expected_bag.head.as_ref() == Some(id) || expected_bag.tail.as_ref() == Some(id);
 		frame_support::ensure!(
diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs
index 355dbea43d699..f3043589681ec 100644
--- a/frame/bags-list/src/list/tests.rs
+++ b/frame/bags-list/src/list/tests.rs
@@ -406,8 +406,8 @@ mod list {
 
 	#[test]
 	fn insert_at_unchecked_at_is_only_node() {
-		// Note that this `insert_at_unchecked` test should fail post checks because node 42 does not
-		// get re-assigned the correct bagu pper. This is because `insert_at_unchecked` assumes
+		// Note that this `insert_at_unchecked` test should fail post checks because node 42 does
+		// not get re-assigned the correct bagu pper. This is because `insert_at_unchecked` assumes
 		// both nodes are already in the same bag with the correct bag upper.
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			// given

From fd751fcdd12bc1b7feb712e4a977b3bc0196b0a6 Mon Sep 17 00:00:00 2001
From: Parity Bot <admin@parity.io>
Date: Wed, 17 Nov 2021 14:13:24 +0000
Subject: [PATCH 23/30] cargo run --quiet --release
 --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml --
 benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_bags_list
 --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096
 --output=./frame/bags-list/src/weights.rs
 --template=./.maintain/frame-weight-template.hbs

---
 frame/bags-list/src/weights.rs | 64 ++++++----------------------------
 1 file changed, 10 insertions(+), 54 deletions(-)

diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index 66bcbb7bf65d6..9efdf5ffff7e1 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -18,7 +18,7 @@
 //! Autogenerated weights for pallet_bags_list
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2021-09-15, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2021-11-17, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
 
 // Executed Command:
@@ -35,7 +35,6 @@
 // --output=./frame/bags-list/src/weights.rs
 // --template=./.maintain/frame-weight-template.hbs
 
-
 #![cfg_attr(rustfmt, rustfmt_skip)]
 #![allow(unused_parens)]
 #![allow(unused_imports)]
@@ -44,59 +43,16 @@ use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
 use sp_std::marker::PhantomData;
 
 /// Weight functions needed for pallet_bags_list.
-pub trait WeightInfo {
-	fn rebag_non_terminal() -> Weight;
-	fn rebag_terminal() -> Weight;
-	fn put_in_front_of() -> Weight;
-}
+pub trait WeightInfo {	fn rebag_non_terminal() -> Weight;	fn rebag_terminal() -> Weight;	fn put_in_front_of() -> Weight;}
 
 /// Weights for pallet_bags_list using the Substrate node and recommended hardware.
-pub struct SubstrateWeight<T>(PhantomData<T>);
-impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
-	// Storage: Staking Bonded (r:1 w:0)
-	// Storage: Staking Ledger (r:1 w:0)
-	// Storage: BagsList ListNodes (r:4 w:4)
-	// Storage: BagsList ListBags (r:1 w:1)
-	fn rebag_non_terminal() -> Weight {
-		(74_175_000 as Weight)
-			.saturating_add(T::DbWeight::get().reads(7 as Weight))
-			.saturating_add(T::DbWeight::get().writes(5 as Weight))
-	}
-	// Storage: Staking Bonded (r:1 w:0)
-	// Storage: Staking Ledger (r:1 w:0)
-	// Storage: BagsList ListNodes (r:3 w:3)
-	// Storage: BagsList ListBags (r:2 w:2)
-	fn rebag_terminal() -> Weight {
-		(73_305_000 as Weight)
-			.saturating_add(T::DbWeight::get().reads(7 as Weight))
-			.saturating_add(T::DbWeight::get().writes(5 as Weight))
-	}
-	fn put_in_front_of() -> Weight {
-		0
-	}
-}
+pub struct SubstrateWeight<T>(PhantomData<T>);impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: BagsList ListBags (r:1 w:1)	fn rebag_non_terminal() -> Weight {
+		(71_536_000 as Weight)			.saturating_add(T::DbWeight::get().reads(7 as Weight))			.saturating_add(T::DbWeight::get().writes(5 as Weight))	}	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:3 w:3)	// Storage: BagsList ListBags (r:2 w:2)	fn rebag_terminal() -> Weight {
+		(68_829_000 as Weight)			.saturating_add(T::DbWeight::get().reads(7 as Weight))			.saturating_add(T::DbWeight::get().writes(5 as Weight))	}	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: Staking Bonded (r:2 w:0)	// Storage: Staking Ledger (r:2 w:0)	// Storage: BagsList CounterForListNodes (r:1 w:1)	// Storage: BagsList ListBags (r:1 w:1)	fn put_in_front_of() -> Weight {
+		(82_771_000 as Weight)			.saturating_add(T::DbWeight::get().reads(10 as Weight))			.saturating_add(T::DbWeight::get().writes(6 as Weight))	}}
 
 // For backwards compatibility and tests
-impl WeightInfo for () {
-	// Storage: Staking Bonded (r:1 w:0)
-	// Storage: Staking Ledger (r:1 w:0)
-	// Storage: BagsList ListNodes (r:4 w:4)
-	// Storage: BagsList ListBags (r:1 w:1)
-	fn rebag_non_terminal() -> Weight {
-		(74_175_000 as Weight)
-			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
-	}
-	// Storage: Staking Bonded (r:1 w:0)
-	// Storage: Staking Ledger (r:1 w:0)
-	// Storage: BagsList ListNodes (r:3 w:3)
-	// Storage: BagsList ListBags (r:2 w:2)
-	fn rebag_terminal() -> Weight {
-		(73_305_000 as Weight)
-			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
-			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
-	}
-	fn put_in_front_of() -> Weight {
-		0
-	}
-}
+impl WeightInfo for () {	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: BagsList ListBags (r:1 w:1)	fn rebag_non_terminal() -> Weight {
+		(71_536_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(7 as Weight))			.saturating_add(RocksDbWeight::get().writes(5 as Weight))	}	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:3 w:3)	// Storage: BagsList ListBags (r:2 w:2)	fn rebag_terminal() -> Weight {
+		(68_829_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(7 as Weight))			.saturating_add(RocksDbWeight::get().writes(5 as Weight))	}	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: Staking Bonded (r:2 w:0)	// Storage: Staking Ledger (r:2 w:0)	// Storage: BagsList CounterForListNodes (r:1 w:1)	// Storage: BagsList ListBags (r:1 w:1)	fn put_in_front_of() -> Weight {
+		(82_771_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(10 as Weight))			.saturating_add(RocksDbWeight::get().writes(6 as Weight))	}}

From f5b02240b377fcaec672f2285a5476df040c6852 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 17 Nov 2021 18:33:32 +0100
Subject: [PATCH 24/30] Delete messed up weights file

---
 frame/bags-list/src/weights.rs | 58 ----------------------------------
 1 file changed, 58 deletions(-)

diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index 9efdf5ffff7e1..e69de29bb2d1d 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -1,58 +0,0 @@
-// This file is part of Substrate.
-
-// Copyright (C) 2021 Parity Technologies (UK) Ltd.
-// SPDX-License-Identifier: Apache-2.0
-
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-//! Autogenerated weights for pallet_bags_list
-//!
-//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2021-11-17, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
-//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
-
-// Executed Command:
-// target/release/substrate
-// benchmark
-// --chain=dev
-// --steps=50
-// --repeat=20
-// --pallet=pallet_bags_list
-// --extrinsic=*
-// --execution=wasm
-// --wasm-execution=compiled
-// --heap-pages=4096
-// --output=./frame/bags-list/src/weights.rs
-// --template=./.maintain/frame-weight-template.hbs
-
-#![cfg_attr(rustfmt, rustfmt_skip)]
-#![allow(unused_parens)]
-#![allow(unused_imports)]
-
-use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
-use sp_std::marker::PhantomData;
-
-/// Weight functions needed for pallet_bags_list.
-pub trait WeightInfo {	fn rebag_non_terminal() -> Weight;	fn rebag_terminal() -> Weight;	fn put_in_front_of() -> Weight;}
-
-/// Weights for pallet_bags_list using the Substrate node and recommended hardware.
-pub struct SubstrateWeight<T>(PhantomData<T>);impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: BagsList ListBags (r:1 w:1)	fn rebag_non_terminal() -> Weight {
-		(71_536_000 as Weight)			.saturating_add(T::DbWeight::get().reads(7 as Weight))			.saturating_add(T::DbWeight::get().writes(5 as Weight))	}	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:3 w:3)	// Storage: BagsList ListBags (r:2 w:2)	fn rebag_terminal() -> Weight {
-		(68_829_000 as Weight)			.saturating_add(T::DbWeight::get().reads(7 as Weight))			.saturating_add(T::DbWeight::get().writes(5 as Weight))	}	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: Staking Bonded (r:2 w:0)	// Storage: Staking Ledger (r:2 w:0)	// Storage: BagsList CounterForListNodes (r:1 w:1)	// Storage: BagsList ListBags (r:1 w:1)	fn put_in_front_of() -> Weight {
-		(82_771_000 as Weight)			.saturating_add(T::DbWeight::get().reads(10 as Weight))			.saturating_add(T::DbWeight::get().writes(6 as Weight))	}}
-
-// For backwards compatibility and tests
-impl WeightInfo for () {	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: BagsList ListBags (r:1 w:1)	fn rebag_non_terminal() -> Weight {
-		(71_536_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(7 as Weight))			.saturating_add(RocksDbWeight::get().writes(5 as Weight))	}	// Storage: Staking Bonded (r:1 w:0)	// Storage: Staking Ledger (r:1 w:0)	// Storage: BagsList ListNodes (r:3 w:3)	// Storage: BagsList ListBags (r:2 w:2)	fn rebag_terminal() -> Weight {
-		(68_829_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(7 as Weight))			.saturating_add(RocksDbWeight::get().writes(5 as Weight))	}	// Storage: BagsList ListNodes (r:4 w:4)	// Storage: Staking Bonded (r:2 w:0)	// Storage: Staking Ledger (r:2 w:0)	// Storage: BagsList CounterForListNodes (r:1 w:1)	// Storage: BagsList ListBags (r:1 w:1)	fn put_in_front_of() -> Weight {
-		(82_771_000 as Weight)			.saturating_add(RocksDbWeight::get().reads(10 as Weight))			.saturating_add(RocksDbWeight::get().writes(6 as Weight))	}}

From 91317bd8a8d580d4bbcf41e62aaeee03fe56a767 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Wed, 17 Nov 2021 18:38:06 +0100
Subject: [PATCH 25/30] Some place holder stuff so we can run bench in CI

---
 frame/bags-list/src/lib.rs     |  7 +++-
 frame/bags-list/src/weights.rs | 70 ++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 1661af9d62270..314947f6a3d83 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -68,7 +68,12 @@ pub mod weights;
 
 pub use list::{notional_bag_for, Bag, Error, List, Node};
 pub use pallet::*;
-pub use weights::WeightInfo;
+
+pub trait WeightInfo {
+	fn rebag_non_terminal() -> Weight;
+	fn rebag_terminal() -> Weight;
+	fn put_in_front_of() -> Weight;
+}
 
 pub(crate) const LOG_TARGET: &'static str = "runtime::bags_list";
 
diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index e69de29bb2d1d..a544873dba6fa 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -0,0 +1,70 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2021 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Autogenerated weights for pallet_bags_list
+//!
+//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
+//! DATE: 2021-11-17, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
+
+// Executed Command:
+// target/release/substrate
+// benchmark
+// --chain=dev
+// --steps=50
+// --repeat=20
+// --pallet=pallet_bags_list
+// --extrinsic=*
+// --execution=wasm
+// --wasm-execution=compiled
+// --heap-pages=4096
+// --output=./frame/bags-list/src/weights.rs
+// --template=./.maintain/frame-weight-template.hbs
+
+#![cfg_attr(rustfmt, rustfmt_skip)]
+#![allow(unused_parens)]
+#![allow(unused_imports)]
+
+use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
+use sp_std::marker::PhantomData;
+
+/// Weights for pallet_bags_list using the Substrate node and recommended hardware.
+pub struct SubstrateWeight<T>(PhantomData<T>);
+impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
+	fn rebag_non_terminal() -> Weight {
+		0
+	}
+	fn rebag_terminal() -> Weight {
+		0
+	}
+	fn put_in_front_of() -> Weight {
+		0
+	}
+}
+
+// For backwards compatibility and tests
+impl WeightInfo for () {
+	fn rebag_non_terminal() -> Weight {
+		0
+	}
+	fn rebag_terminal() -> Weight {
+		0
+	}
+	fn put_in_front_of() -> Weight {
+		0
+	}
+}
\ No newline at end of file

From 6af64cea822bc271f7850999af6a5f3f39b576b7 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Thu, 18 Nov 2021 10:55:14 +0100
Subject: [PATCH 26/30] Fix

---
 frame/bags-list/src/lib.rs     | 7 +------
 frame/bags-list/src/weights.rs | 6 ++++++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs
index 314947f6a3d83..1661af9d62270 100644
--- a/frame/bags-list/src/lib.rs
+++ b/frame/bags-list/src/lib.rs
@@ -68,12 +68,7 @@ pub mod weights;
 
 pub use list::{notional_bag_for, Bag, Error, List, Node};
 pub use pallet::*;
-
-pub trait WeightInfo {
-	fn rebag_non_terminal() -> Weight;
-	fn rebag_terminal() -> Weight;
-	fn put_in_front_of() -> Weight;
-}
+pub use weights::WeightInfo;
 
 pub(crate) const LOG_TARGET: &'static str = "runtime::bags_list";
 
diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index a544873dba6fa..15a259a2043d3 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -42,6 +42,12 @@
 use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
 use sp_std::marker::PhantomData;
 
+pub trait WeightInfo {
+	fn rebag_non_terminal() -> Weight;
+	fn rebag_terminal() -> Weight;
+	fn put_in_front_of() -> Weight;
+}
+
 /// Weights for pallet_bags_list using the Substrate node and recommended hardware.
 pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {

From 56ea8cc59cb9e0997ad566caf0ee03f7dbd98111 Mon Sep 17 00:00:00 2001
From: Parity Bot <admin@parity.io>
Date: Thu, 18 Nov 2021 10:30:42 +0000
Subject: [PATCH 27/30] cargo run --quiet --release
 --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml --
 benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_bags_list
 --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096
 --output=./frame/bags-list/src/weights.rs
 --template=./.maintain/frame-weight-template.hbs

---
 frame/bags-list/src/weights.rs | 55 +++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

diff --git a/frame/bags-list/src/weights.rs b/frame/bags-list/src/weights.rs
index 15a259a2043d3..7433c7ac564f7 100644
--- a/frame/bags-list/src/weights.rs
+++ b/frame/bags-list/src/weights.rs
@@ -18,7 +18,7 @@
 //! Autogenerated weights for pallet_bags_list
 //!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
-//! DATE: 2021-11-17, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
+//! DATE: 2021-11-18, STEPS: `50`, REPEAT: 20, LOW RANGE: `[]`, HIGH RANGE: `[]`
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
 
 // Executed Command:
@@ -42,6 +42,7 @@
 use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};
 use sp_std::marker::PhantomData;
 
+/// Weight functions needed for pallet_bags_list.
 pub trait WeightInfo {
 	fn rebag_non_terminal() -> Weight;
 	fn rebag_terminal() -> Weight;
@@ -51,26 +52,64 @@ pub trait WeightInfo {
 /// Weights for pallet_bags_list using the Substrate node and recommended hardware.
 pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
+	// Storage: Staking Bonded (r:1 w:0)
+	// Storage: Staking Ledger (r:1 w:0)
+	// Storage: BagsList ListNodes (r:4 w:4)
+	// Storage: BagsList ListBags (r:1 w:1)
 	fn rebag_non_terminal() -> Weight {
-		0
+		(70_481_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(7 as Weight))
+			.saturating_add(T::DbWeight::get().writes(5 as Weight))
 	}
+	// Storage: Staking Bonded (r:1 w:0)
+	// Storage: Staking Ledger (r:1 w:0)
+	// Storage: BagsList ListNodes (r:3 w:3)
+	// Storage: BagsList ListBags (r:2 w:2)
 	fn rebag_terminal() -> Weight {
-		0
+		(68_642_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(7 as Weight))
+			.saturating_add(T::DbWeight::get().writes(5 as Weight))
 	}
+	// Storage: BagsList ListNodes (r:4 w:4)
+	// Storage: Staking Bonded (r:2 w:0)
+	// Storage: Staking Ledger (r:2 w:0)
+	// Storage: BagsList CounterForListNodes (r:1 w:1)
+	// Storage: BagsList ListBags (r:1 w:1)
 	fn put_in_front_of() -> Weight {
-		0
+		(82_341_000 as Weight)
+			.saturating_add(T::DbWeight::get().reads(10 as Weight))
+			.saturating_add(T::DbWeight::get().writes(6 as Weight))
 	}
 }
 
 // For backwards compatibility and tests
 impl WeightInfo for () {
+	// Storage: Staking Bonded (r:1 w:0)
+	// Storage: Staking Ledger (r:1 w:0)
+	// Storage: BagsList ListNodes (r:4 w:4)
+	// Storage: BagsList ListBags (r:1 w:1)
 	fn rebag_non_terminal() -> Weight {
-		0
+		(70_481_000 as Weight)
+			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
 	}
+	// Storage: Staking Bonded (r:1 w:0)
+	// Storage: Staking Ledger (r:1 w:0)
+	// Storage: BagsList ListNodes (r:3 w:3)
+	// Storage: BagsList ListBags (r:2 w:2)
 	fn rebag_terminal() -> Weight {
-		0
+		(68_642_000 as Weight)
+			.saturating_add(RocksDbWeight::get().reads(7 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
 	}
+	// Storage: BagsList ListNodes (r:4 w:4)
+	// Storage: Staking Bonded (r:2 w:0)
+	// Storage: Staking Ledger (r:2 w:0)
+	// Storage: BagsList CounterForListNodes (r:1 w:1)
+	// Storage: BagsList ListBags (r:1 w:1)
 	fn put_in_front_of() -> Weight {
-		0
+		(82_341_000 as Weight)
+			.saturating_add(RocksDbWeight::get().reads(10 as Weight))
+			.saturating_add(RocksDbWeight::get().writes(6 as Weight))
 	}
-}
\ No newline at end of file
+}

From 77fd777b6f8701afdff24de6e4bfaa725a75d7c6 Mon Sep 17 00:00:00 2001
From: Zeke Mostov <z.mostov@gmail.com>
Date: Mon, 6 Dec 2021 14:42:45 -0800
Subject: [PATCH 28/30] Update frame/bags-list/src/list/mod.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
---
 frame/bags-list/src/list/mod.rs | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index bce4c0f3e022b..3dd072724cb29 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -430,8 +430,8 @@ impl<T: Config> List<T> {
 		node.next = Some(at.id().clone());
 		at.prev = Some(node.id().clone());
 
-		if at.is_terminal() || node.is_terminal() {
-			// `at` is the tail and/or `node` is the head, so we make sure the bag is updated. Note,
+		if node.is_terminal() {
+			// `node` is the new head, so we make sure the bag is updated. Note,
 			// since `node` is always in front of `at` we know that 1) there is always at least 2
 			// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
 			// tail.
@@ -442,13 +442,10 @@ impl<T: Config> List<T> {
 				bag.head = Some(node.id().clone())
 			}
 
-			if at.next == None {
-				bag.tail = Some(at.id().clone())
-			}
-
 			bag.put()
 		};
 
+
 		// write the updated nodes to storage.
 		at.put();
 		node.put();

From aa4d8961d331cecfa1dcfa757955ee508674f491 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Mon, 6 Dec 2021 20:13:42 -0800
Subject: [PATCH 29/30] fmt

---
 frame/bags-list/src/list/mod.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 3dd072724cb29..1789a81ce0d6f 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -445,7 +445,6 @@ impl<T: Config> List<T> {
 			bag.put()
 		};
 
-
 		// write the updated nodes to storage.
 		at.put();
 		node.put();

From 944dc85071d5aebc1fcfd43d5b456d4334046239 Mon Sep 17 00:00:00 2001
From: emostov <32168567+emostov@users.noreply.github.com>
Date: Mon, 6 Dec 2021 20:29:23 -0800
Subject: [PATCH 30/30] Log + debug assert when refetching an Id

---
 frame/bags-list/src/list/mod.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs
index 1789a81ce0d6f..a84cffe57b475 100644
--- a/frame/bags-list/src/list/mod.rs
+++ b/frame/bags-list/src/list/mod.rs
@@ -404,7 +404,12 @@ impl<T: Config> List<T> {
 
 		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
 		// was removed.
-		let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
+		let lighter_node = Node::<T>::get(&lighter_id).ok_or_else(|| {
+			debug_assert!(false, "id that should exist cannot be found");
+			crate::log!(warn, "id that should exist cannot be found");
+			pallet::Error::IdNotFound
+		})?;
+
 		// insert `heavier_node` directly in front of `lighter_node`. This will update both nodes
 		// in storage and update the node counter.
 		Self::insert_at_unchecked(lighter_node, heavier_node);