Improve the debug asserts in remove_node

frame/staking/src/mock/voter_bags.rs

@@ -31,7 +31,7 @@ pub const EXISTENTIAL_WEIGHT: u64 = 1;
 pub const CONSTANT_RATIO: f64 = 2.0000000000000000;
 
 /// Upper thresholds delimiting the bag list.
-pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]
+// pub const THRESHOLDS: [u64; 65] = [5, 15, 25, 50, 100, 1000, 2000, 3000]; // TODO: switch to this
 pub const THRESHOLDS: [u64; 65] = [
 	                         1,
 	                         2,

frame/staking/src/tests.rs

@@ -153,7 +153,7 @@ fn basic_setup_works() {
 		// for these stash ids, see
 		// https://github.com/paritytech/substrate/
 		//   blob/631d4cdbcad438248c2597213918d8207d85bf6e/frame/staking/src/mock.rs#L435-L441
-		for genesis_stash_account_id in [11, 21, 31, 101] {
+		for genesis_stash_account_id in &[11, 21, 31, 101] {
 			let node = crate::voter_bags::Node::<Test>::from_id(&genesis_stash_account_id)
 				.expect(&format!("node was created for account {}", genesis_stash_account_id));
 			assert!(!node.is_misplaced(&weight_of));

frame/staking/src/voter_bags.rs

@@ -26,7 +26,6 @@ use frame_support::{traits::Get, DefaultNoBound};
 use sp_runtime::SaturatedConversion;
 use sp_std::{
 	boxed::Box,
-	cmp::PartialEq,
 	collections::{btree_map::BTreeMap, btree_set::BTreeSet},
 	iter,
 	marker::PhantomData,
@@ -464,20 +463,43 @@ impl<T: Config> Bag<T> {
 	fn remove_node(&mut self, node: &Node<T>) {
 		// Excise `node`.
 		if let Some(mut prev) = node.prev() {
-			debug_assert!(prev != *node, "node.prev circularly points at node");
+			debug_assert!(prev.voter.id != node.voter.id, "node.prev circularly points at node");
+			debug_assert!(
+				self.head.as_ref() != Some(&node.voter.id),
+				"node is the head, but has Some prev"
+			);
 
 			prev.next = node.next.clone();
-			debug_assert!(prev.next().unwrap().prev().unwrap() == *node);
+			debug_assert!(
+				prev.next().and_then(|prev_next|
+					// prev.next.prev should should point at node prior to being reassigned
+					Some(prev_next.prev().unwrap().voter.id == node.voter.id)
+				)
+				// unless prev.next is None, in which case node has to be the tail
+				.unwrap_or(self.tail.as_ref() == Some(&node.voter.id)),
+				"prev.next.prev should should point at node prior to being reassigned (if node is not the tail)"
+			);
 
 			prev.put();
 		}
 		if let Some(mut next) = node.next() {
 			// if there is a node.next, we know their should be a node.prev
-			debug_assert!(next != *node, "node.next circularly points at node");
-			debug_assert!(node.prev().unwrap() != next, "node.prev circularly points at node.next");
+			debug_assert!(next.voter.id != node.voter.id, "node.next circularly points at node");
+			debug_assert!(
+				self.tail.as_ref() != Some(&node.voter.id),
+				"node is the tail, but has Some next"
+			);
 
 			next.prev = node.prev.clone();
-			debug_assert!(next.prev().unwrap().next().unwrap() == next);
+			debug_assert!(
+				next.prev().and_then(|next_prev|
+					// next.prev.next should point at next after being reassigned
+					Some(next_prev.next().unwrap().voter.id == next.voter.id)
+				)
+				// unless next.prev is None, in which case node has to be the head
+				.unwrap_or_else(|| self.head.as_ref() == Some(&node.voter.id)),
+				"next.prev.next should point at next after being reassigned (if node is not the head)"
+			);
 
 			next.put();
 		}
@@ -494,7 +516,7 @@ impl<T: Config> Bag<T> {
 
 /// A Node is the fundamental element comprising the doubly-linked lists which for each bag.
 #[derive(Encode, Decode)]
-#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, PartialEq))]
+#[cfg_attr(feature = "std", derive(frame_support::DebugNoBound))]
 pub struct Node<T: Config> {
 	voter: Voter<AccountIdOf<T>>,
 	prev: Option<AccountIdOf<T>>,