Try-state for Collective pallet (#13645)

* write the try_state_ function

* update tests

* update check

* fix benchmarking

* fix nonsense

* Update frame/collective/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/collective/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* unique proposal index

* prime must be a member of the collective

* oops

* Add new checks

* use ensure

* fix

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/collective/src/benchmarking.rs

@@ -646,5 +646,5 @@ benchmarks_instance_pallet! {
 		assert_last_event::<T, I>(Event::Disapproved { proposal_hash: last_hash }.into());
 	}
 
-	impl_benchmark_test_suite!(Collective, crate::tests::new_test_ext(), crate::tests::Test);
+	impl_benchmark_test_suite!(Collective, crate::tests::ExtBuilder::default().build(), crate::tests::Test);
 }

substrate/frame/collective/src/lib.rs

@@ -50,8 +50,8 @@ use sp_std::{marker::PhantomData, prelude::*, result};
 use frame_support::{
 	codec::{Decode, Encode, MaxEncodedLen},
 	dispatch::{
-		DispatchError, DispatchResultWithPostInfo, Dispatchable, GetDispatchInfo, Pays,
-		PostDispatchInfo,
+		DispatchError, DispatchResult, DispatchResultWithPostInfo, Dispatchable, GetDispatchInfo,
+		Pays, PostDispatchInfo,
 	},
 	ensure,
 	traits::{
@@ -341,6 +341,15 @@ pub mod pallet {
 		WrongProposalLength,
 	}
 
+	#[pallet::hooks]
+	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
+		#[cfg(feature = "try-runtime")]
+		fn try_state(_n: BlockNumberFor<T>) -> Result<(), &'static str> {
+			Self::do_try_state()?;
+			Ok(())
+		}
+	}
+
 	// Note that councillor operations are assigned to the operational class.
 	#[pallet::call]
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -916,6 +925,111 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		});
 		num_proposals as u32
 	}
+
+	/// Ensure the correctness of the state of this pallet.
+	///
+	/// The following expectation must always apply.
+	///
+	/// ## Expectations:
+	///
+	/// Looking at proposals:
+	///
+	/// * Each hash of a proposal that is stored inside `Proposals` must have a
+	/// call mapped to it inside the `ProposalOf` storage map.
+	/// * `ProposalCount` must always be more or equal to the number of
+	/// proposals inside the `Proposals` storage value. The reason why
+	/// `ProposalCount` can be more is because when a proposal is removed the
+	/// count is not deducted.
+	/// * Count of `ProposalOf` should match the count of `Proposals`
+	///
+	/// Looking at votes:
+	/// * The sum of aye and nay votes for a proposal can never exceed
+	///  `MaxMembers`.
+	/// * The proposal index inside the `Voting` storage map must be unique.
+	/// * All proposal hashes inside `Voting` must exist in `Proposals`.
+	///
+	/// Looking at members:
+	/// * The members count must never exceed `MaxMembers`.
+	/// * All the members must be sorted by value.
+	///
+	/// Looking at prime account:
+	/// * The prime account must be a member of the collective.
+	#[cfg(any(feature = "try-runtime", test))]
+	fn do_try_state() -> DispatchResult {
+		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
+			ensure!(
+				Self::proposal_of(proposal).is_some(),
+				DispatchError::Other(
+					"Proposal hash from `Proposals` is not found inside the `ProposalOf` mapping."
+				)
+			);
+			Ok(())
+		})?;
+
+		ensure!(
+			Self::proposals().into_iter().count() <= Self::proposal_count() as usize,
+			DispatchError::Other("The actual number of proposals is greater than `ProposalCount`")
+		);
+		ensure!(
+			Self::proposals().into_iter().count() == <ProposalOf<T, I>>::iter_keys().count(),
+			DispatchError::Other("Proposal count inside `Proposals` is not equal to the proposal count in `ProposalOf`")
+		);
+
+		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
+			if let Some(votes) = Self::voting(proposal) {
+				let ayes = votes.ayes.len();
+				let nays = votes.nays.len();
+
+				ensure!(
+					ayes.saturating_add(nays) <= T::MaxMembers::get() as usize,
+					DispatchError::Other("The sum of ayes and nays is greater than `MaxMembers`")
+				);
+			}
+			Ok(())
+		})?;
+
+		let mut proposal_indices = vec![];
+		Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult {
+			if let Some(votes) = Self::voting(proposal) {
+				let proposal_index = votes.index;
+				ensure!(
+					!proposal_indices.contains(&proposal_index),
+					DispatchError::Other("The proposal index is not unique.")
+				);
+				proposal_indices.push(proposal_index);
+			}
+			Ok(())
+		})?;
+
+		<Voting<T, I>>::iter_keys().try_for_each(|proposal_hash| -> DispatchResult {
+			ensure!(
+				Self::proposals().contains(&proposal_hash),
+				DispatchError::Other(
+					"`Proposals` doesn't contain the proposal hash from the `Voting` storage map."
+				)
+			);
+			Ok(())
+		})?;
+
+		ensure!(
+			Self::members().len() <= T::MaxMembers::get() as usize,
+			DispatchError::Other("The member count is greater than `MaxMembers`.")
+		);
+
+		ensure!(
+			Self::members().windows(2).all(|members| members[0] <= members[1]),
+			DispatchError::Other("The members are not sorted by value.")
+		);
+
+		if let Some(prime) = Self::prime() {
+			ensure!(
+				Self::members().contains(&prime),
+				DispatchError::Other("Prime account is not a member.")
+			);
+		}
+
+		Ok(())
+	}
 }
 
 impl<T: Config<I>, I: 'static> ChangeMembers<T::AccountId> for Pallet<T, I> {

substrate/frame/collective/src/tests.rs

@@ -155,23 +155,40 @@ impl Config for Test {
 	type SetMembersOrigin = EnsureRoot<Self::AccountId>;
 }
 
-pub fn new_test_ext() -> sp_io::TestExternalities {
-	let mut ext: sp_io::TestExternalities = GenesisConfig {
-		collective: pallet_collective::GenesisConfig {
-			members: vec![1, 2, 3],
-			phantom: Default::default(),
-		},
-		collective_majority: pallet_collective::GenesisConfig {
-			members: vec![1, 2, 3, 4, 5],
-			phantom: Default::default(),
-		},
-		default_collective: Default::default(),
+pub struct ExtBuilder {}
+
+impl Default for ExtBuilder {
+	fn default() -> Self {
+		Self {}
+	}
+}
+
+impl ExtBuilder {
+	pub fn build(self) -> sp_io::TestExternalities {
+		let mut ext: sp_io::TestExternalities = GenesisConfig {
+			collective: pallet_collective::GenesisConfig {
+				members: vec![1, 2, 3],
+				phantom: Default::default(),
+			},
+			collective_majority: pallet_collective::GenesisConfig {
+				members: vec![1, 2, 3, 4, 5],
+				phantom: Default::default(),
+			},
+			default_collective: Default::default(),
+		}
+		.build_storage()
+		.unwrap()
+		.into();
+		ext.execute_with(|| System::set_block_number(1));
+		ext
+	}
+
+	pub fn build_and_execute(self, test: impl FnOnce() -> ()) {
+		self.build().execute_with(|| {
+			test();
+			Collective::do_try_state().unwrap();
+		})
 	}
-	.build_storage()
-	.unwrap()
-	.into();
-	ext.execute_with(|| System::set_block_number(1));
-	ext
 }
 
 fn make_proposal(value: u64) -> RuntimeCall {
@@ -186,7 +203,7 @@ fn record(event: RuntimeEvent) -> EventRecord<RuntimeEvent, H256> {
 
 #[test]
 fn motions_basic_environment_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		assert_eq!(Collective::members(), vec![1, 2, 3]);
 		assert_eq!(*Collective::proposals(), Vec::<H256>::new());
 	});
@@ -194,7 +211,7 @@ fn motions_basic_environment_works() {
 
 #[test]
 fn close_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -262,7 +279,7 @@ fn close_works() {
 
 #[test]
 fn proposal_weight_limit_works_on_approve() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = RuntimeCall::Collective(crate::Call::set_members {
 			new_members: vec![1, 2, 3],
 			prime: None,
@@ -304,7 +321,7 @@ fn proposal_weight_limit_works_on_approve() {
 
 #[test]
 fn proposal_weight_limit_ignored_on_disapprove() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = RuntimeCall::Collective(crate::Call::set_members {
 			new_members: vec![1, 2, 3],
 			prime: None,
@@ -334,7 +351,7 @@ fn proposal_weight_limit_ignored_on_disapprove() {
 
 #[test]
 fn close_with_prime_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -402,7 +419,7 @@ fn close_with_prime_works() {
 
 #[test]
 fn close_with_voting_prime_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -472,7 +489,7 @@ fn close_with_voting_prime_works() {
 
 #[test]
 fn close_with_no_prime_but_majority_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -552,7 +569,7 @@ fn close_with_no_prime_but_majority_works() {
 
 #[test]
 fn removal_of_old_voters_votes_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash = BlakeTwo256::hash_of(&proposal);
@@ -600,7 +617,7 @@ fn removal_of_old_voters_votes_works() {
 
 #[test]
 fn removal_of_old_voters_votes_works_with_set_members() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash = BlakeTwo256::hash_of(&proposal);
@@ -658,7 +675,7 @@ fn removal_of_old_voters_votes_works_with_set_members() {
 
 #[test]
 fn propose_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash = proposal.blake2_256().into();
@@ -690,7 +707,7 @@ fn propose_works() {
 
 #[test]
 fn limit_active_proposals() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		for i in 0..MaxProposals::get() {
 			let proposal = make_proposal(i as u64);
 			let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
@@ -717,7 +734,7 @@ fn limit_active_proposals() {
 
 #[test]
 fn correct_validate_and_get_proposal() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = RuntimeCall::Collective(crate::Call::set_members {
 			new_members: vec![1, 2, 3],
 			prime: None,
@@ -763,7 +780,7 @@ fn correct_validate_and_get_proposal() {
 
 #[test]
 fn motions_ignoring_non_collective_proposals_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		assert_noop!(
@@ -780,7 +797,7 @@ fn motions_ignoring_non_collective_proposals_works() {
 
 #[test]
 fn motions_ignoring_non_collective_votes_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash: H256 = proposal.blake2_256().into();
@@ -799,7 +816,7 @@ fn motions_ignoring_non_collective_votes_works() {
 
 #[test]
 fn motions_ignoring_bad_index_collective_vote_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		System::set_block_number(3);
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
@@ -819,7 +836,7 @@ fn motions_ignoring_bad_index_collective_vote_works() {
 
 #[test]
 fn motions_vote_after_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash: H256 = proposal.blake2_256().into();
@@ -888,7 +905,7 @@ fn motions_vote_after_works() {
 
 #[test]
 fn motions_all_first_vote_free_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash: H256 = proposal.blake2_256().into();
@@ -946,7 +963,7 @@ fn motions_all_first_vote_free_works() {
 
 #[test]
 fn motions_reproposing_disapproved_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -978,7 +995,7 @@ fn motions_reproposing_disapproved_works() {
 
 #[test]
 fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = RuntimeCall::Democracy(mock_democracy::Call::external_propose_majority {});
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -1108,7 +1125,7 @@ fn motions_approval_with_enough_votes_and_lower_voting_threshold_works() {
 
 #[test]
 fn motions_disapproval_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -1167,7 +1184,7 @@ fn motions_disapproval_works() {
 
 #[test]
 fn motions_approval_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -1228,7 +1245,7 @@ fn motions_approval_works() {
 
 #[test]
 fn motion_with_no_votes_closes_with_disapproval() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let proposal_weight = proposal.get_dispatch_info().weight;
@@ -1289,7 +1306,7 @@ fn close_disapprove_does_not_care_about_weight_or_len() {
 	// This test confirms that if you close a proposal that would be disapproved,
 	// we do not care about the proposal length or proposal weight since it will
 	// not be read from storage or executed.
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash: H256 = proposal.blake2_256().into();
@@ -1321,7 +1338,7 @@ fn close_disapprove_does_not_care_about_weight_or_len() {
 
 #[test]
 fn disapprove_proposal_works() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		let proposal = make_proposal(42);
 		let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
 		let hash: H256 = proposal.blake2_256().into();
@@ -1380,7 +1397,7 @@ fn genesis_build_panics_with_duplicate_members() {
 
 #[test]
 fn migration_v4() {
-	new_test_ext().execute_with(|| {
+	ExtBuilder::default().build_and_execute(|| {
 		use frame_support::traits::PalletInfoAccess;
 
 		let old_pallet = "OldCollective";