Runtime State Test + Integration with try-runtime (#10174)

* add missing version to dependencies

* Huh

* add features more

* more fixing

* last touches

* it all finally works

* remove some feature gates

* remove unused

* fix old macro

* make it work again

* fmt

* remove unused import

* ".git/.scripts/fmt.sh" 1

* Cleanup more

* fix and rename everything

* a few clippy fixes

* Add try-runtime feature

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* small fixes

* fmt

* Update bin/node-template/runtime/src/lib.rs

* fix build

* Update utils/frame/try-runtime/cli/src/lib.rs

Co-authored-by: David <dvdplm@gmail.com>

* Update utils/frame/try-runtime/cli/src/commands/execute_block.rs

Co-authored-by: David <dvdplm@gmail.com>

* address all review comments

* fix typos

* revert spec change

* last touches

* update docs

* fmt

* remove some debug_assertions

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: David <dvdplm@gmail.com>

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

@@ -88,7 +88,7 @@ fn main() {
 				},
 			}
 
-			assert!(BagsList::sanity_check().is_ok());
+			assert!(BagsList::try_state().is_ok());
 		})
 	});
 }

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

@@ -24,8 +24,8 @@ use sp_std::prelude::*;
 pub const LOG_TARGET: &str = "runtime::bags-list::remote-tests";
 
 pub mod migration;
-pub mod sanity_check;
 pub mod snapshot;
+pub mod try_state;
 
 /// A wrapper for a runtime that the functions of this crate expect.
 ///

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

@@ -44,7 +44,7 @@ pub async fn execute<Runtime: crate::RuntimeT, Block: BlockT + DeserializeOwned>
 
 	ext.execute_with(|| {
 		sp_core::crypto::set_default_ss58_version(Runtime::SS58Prefix::get().try_into().unwrap());
-		pallet_bags_list::Pallet::<Runtime>::sanity_check().unwrap();
+		pallet_bags_list::Pallet::<Runtime>::try_state().unwrap();
 		log::info!(target: crate::LOG_TARGET, "executed bags-list sanity check with no errors.");
 
 		crate::display_and_check_bags::<Runtime>(currency_unit, currency_name);

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

@@ -263,6 +263,11 @@ pub mod pallet {
 				"thresholds must strictly increase, and have no duplicates",
 			);
 		}
+
+		#[cfg(feature = "try-runtime")]
+		fn try_state(_: BlockNumberFor<T>) -> Result<(), &'static str> {
+			<Self as SortedListProvider<T::AccountId>>::try_state()
+		}
 	}
 }
 
@@ -340,14 +345,8 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		List::<T, I>::unsafe_regenerate(all, score_of)
 	}
 
-	#[cfg(feature = "std")]
-	fn sanity_check() -> Result<(), &'static str> {
-		List::<T, I>::sanity_check()
-	}
-
-	#[cfg(not(feature = "std"))]
-	fn sanity_check() -> Result<(), &'static str> {
-		Ok(())
+	fn try_state() -> Result<(), &'static str> {
+		List::<T, I>::try_state()
 	}
 
 	fn unsafe_clear() {

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

@@ -28,8 +28,8 @@ use crate::Config;
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_election_provider_support::ScoreProvider;
 use frame_support::{
-	ensure,
-	traits::{Defensive, Get},
+	defensive, ensure,
+	traits::{Defensive, DefensiveOption, Get},
 	DefaultNoBound, PalletError,
 };
 use scale_info::TypeInfo;
@@ -220,7 +220,8 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 			crate::ListBags::<T, I>::remove(removed_bag);
 		}
 
-		debug_assert_eq!(Self::sanity_check(), Ok(()));
+		#[cfg(feature = "std")]
+		debug_assert_eq!(Self::try_state(), Ok(()));
 
 		num_affected
 	}
@@ -325,8 +326,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 
 		crate::log!(
 			debug,
-			"inserted {:?} with score {:?
-			} into bag {:?}, new count is {}",
+			"inserted {:?} with score {:?} into bag {:?}, new count is {}",
 			id,
 			score,
 			bag_score,
@@ -457,11 +457,8 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 
 		// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
 		// was removed.
-		let lighter_node = Node::<T, I>::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");
-			ListError::NodeNotFound
-		})?;
+		let lighter_node =
+			Node::<T, I>::get(lighter_id).defensive_ok_or_else(|| ListError::NodeNotFound)?;
 
 		// insert `heavier_node` directly in front of `lighter_node`. This will update both nodes
 		// in storage and update the node counter.
@@ -508,7 +505,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		node.put();
 	}
 
-	/// Sanity check the list.
+	/// Check the internal state of the list.
 	///
 	/// This should be called from the call-site, whenever one of the mutating apis (e.g. `insert`)
 	/// is being used, after all other staking data (such as counter) has been updated. It checks:
@@ -517,8 +514,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 	/// * length of this list is in sync with `ListNodes::count()`,
 	/// * and sanity-checks all bags and nodes. This will cascade down all the checks and makes sure
 	/// all bags and nodes are checked per *any* update to `List`.
-	#[cfg(feature = "std")]
-	pub(crate) fn sanity_check() -> Result<(), &'static str> {
+	pub(crate) fn try_state() -> Result<(), &'static str> {
 		let mut seen_in_list = BTreeSet::new();
 		ensure!(
 			Self::iter().map(|node| node.id).all(|id| seen_in_list.insert(id)),
@@ -546,7 +542,7 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 			thresholds.into_iter().filter_map(|t| Bag::<T, I>::get(t))
 		};
 
-		let _ = active_bags.clone().try_for_each(|b| b.sanity_check())?;
+		let _ = active_bags.clone().try_for_each(|b| b.try_state())?;
 
 		let nodes_in_bags_count =
 			active_bags.clone().fold(0u32, |acc, cur| acc + cur.iter().count() as u32);
@@ -557,17 +553,12 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 		// check that all nodes are sane. We check the `ListNodes` storage item directly in case we
 		// have some "stale" nodes that are not in a bag.
 		for (_id, node) in crate::ListNodes::<T, I>::iter() {
-			node.sanity_check()?
+			node.try_state()?
 		}
 
 		Ok(())
 	}
 
-	#[cfg(not(feature = "std"))]
-	pub(crate) fn sanity_check() -> Result<(), &'static str> {
-		Ok(())
-	}
-
 	/// Returns the nodes of all non-empty bags. For testing and benchmarks.
 	#[cfg(any(feature = "std", feature = "runtime-benchmarks"))]
 	#[allow(dead_code)]
@@ -701,8 +692,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 			if *tail == node.id {
 				// this should never happen, but this check prevents one path to a worst case
 				// 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");
+				defensive!("system logic error: inserting a node who has the id of tail");
 				return
 			};
 		}
@@ -753,7 +743,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 		}
 	}
 
-	/// Sanity check this bag.
+	/// Check the internal state of the bag.
 	///
 	/// Should be called by the call-site, after any mutating operation on a bag. The call site of
 	/// this struct is always `List`.
@@ -761,8 +751,7 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 	/// * Ensures head has no prev.
 	/// * Ensures tail has no next.
 	/// * Ensures there are no loops, traversal from head to tail is correct.
-	#[cfg(feature = "std")]
-	fn sanity_check(&self) -> Result<(), &'static str> {
+	fn try_state(&self) -> Result<(), &'static str> {
 		frame_support::ensure!(
 			self.head()
 				.map(|head| head.prev().is_none())
@@ -801,7 +790,6 @@ impl<T: Config<I>, I: 'static> Bag<T, I> {
 	}
 
 	/// Check if the bag contains a node with `id`.
-	#[cfg(feature = "std")]
 	fn contains(&self, id: &T::AccountId) -> bool {
 		self.iter().any(|n| n.id() == id)
 	}
@@ -906,8 +894,7 @@ impl<T: Config<I>, I: 'static> Node<T, I> {
 		self.bag_upper
 	}
 
-	#[cfg(feature = "std")]
-	fn sanity_check(&self) -> Result<(), &'static str> {
+	fn try_state(&self) -> Result<(), &'static str> {
 		let expected_bag = Bag::<T, I>::get(self.bag_upper).ok_or("bag not found for node")?;
 
 		let id = self.id();

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

@@ -350,15 +350,15 @@ mod list {
 	}
 
 	#[test]
-	fn sanity_check_works() {
+	fn try_state_works() {
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
-			assert_ok!(List::<Runtime>::sanity_check());
+			assert_ok!(List::<Runtime>::try_state());
 		});
 
 		// make sure there are no duplicates.
 		ExtBuilder::default().build_and_execute_no_post_check(|| {
 			Bag::<Runtime>::get(10).unwrap().insert_unchecked(2, 10);
-			assert_eq!(List::<Runtime>::sanity_check(), Err("duplicate identified"));
+			assert_eq!(List::<Runtime>::try_state(), Err("duplicate identified"));
 		});
 
 		// ensure count is in sync with `ListNodes::count()`.
@@ -372,7 +372,7 @@ mod list {
 			CounterForListNodes::<Runtime>::mutate(|counter| *counter += 1);
 			assert_eq!(crate::ListNodes::<Runtime>::count(), 5);
 
-			assert_eq!(List::<Runtime>::sanity_check(), Err("iter_count != stored_count"));
+			assert_eq!(List::<Runtime>::try_state(), Err("iter_count != stored_count"));
 		});
 	}
 
@@ -804,7 +804,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 13, 14]);
-				assert_ok!(bag_1000.sanity_check());
+				assert_ok!(bag_1000.try_state());
 				// and the node isn't mutated when its removed
 				assert_eq!(node_4, node_4_pre_remove);
 
@@ -814,7 +814,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_1000), vec![3, 13, 14]);
-				assert_ok!(bag_1000.sanity_check());
+				assert_ok!(bag_1000.try_state());
 
 				// when removing a tail that is not pointing at the head
 				let node_14 = Node::<Runtime>::get(&14).unwrap();
@@ -822,7 +822,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_1000), vec![3, 13]);
-				assert_ok!(bag_1000.sanity_check());
+				assert_ok!(bag_1000.try_state());
 
 				// when removing a tail that is pointing at the head
 				let node_13 = Node::<Runtime>::get(&13).unwrap();
@@ -830,7 +830,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_1000), vec![3]);
-				assert_ok!(bag_1000.sanity_check());
+				assert_ok!(bag_1000.try_state());
 
 				// when removing a node that is both the head & tail
 				let node_3 = Node::<Runtime>::get(&3).unwrap();
@@ -846,7 +846,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_10), vec![1, 12]);
-				assert_ok!(bag_10.sanity_check());
+				assert_ok!(bag_10.try_state());
 
 				// when removing a head that is pointing at the tail
 				let node_1 = Node::<Runtime>::get(&1).unwrap();
@@ -854,7 +854,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_10), vec![12]);
-				assert_ok!(bag_10.sanity_check());
+				assert_ok!(bag_10.try_state());
 				// and since we updated the bag's head/tail, we need to write this storage so we
 				// can correctly `get` it again in later checks
 				bag_10.put();
@@ -865,7 +865,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_2000), vec![15, 17, 18, 19]);
-				assert_ok!(bag_2000.sanity_check());
+				assert_ok!(bag_2000.try_state());
 
 				// when removing a node that is pointing at tail, but not head
 				let node_18 = Node::<Runtime>::get(&18).unwrap();
@@ -873,7 +873,7 @@ mod bags {
 
 				// then
 				assert_eq!(bag_as_ids(&bag_2000), vec![15, 17, 19]);
-				assert_ok!(bag_2000.sanity_check());
+				assert_ok!(bag_2000.try_state());
 
 				// finally, when reading from storage, the state of all bags is as expected
 				assert_eq!(
@@ -905,7 +905,7 @@ mod bags {
 			// .. and the bag it was removed from
 			let bag_1000 = Bag::<Runtime>::get(1_000).unwrap();
 			// is sane
-			assert_ok!(bag_1000.sanity_check());
+			assert_ok!(bag_1000.try_state());
 			// and has the correct head and tail.
 			assert_eq!(bag_1000.head, Some(3));
 			assert_eq!(bag_1000.tail, Some(4));

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

@@ -147,7 +147,7 @@ impl ExtBuilder {
 	pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
 		self.build().execute_with(|| {
 			test();
-			List::<Runtime>::sanity_check().expect("Sanity check post condition failed")
+			List::<Runtime>::try_state().expect("Try-state post condition failed")
 		})
 	}