From 449935fa77283add49700a9956010eb39a6baba3 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sun, 19 Mar 2023 19:32:59 +0100 Subject: [PATCH 01/13] write the try_state_ function --- frame/collective/src/lib.rs | 46 +++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index ae5b22559d7c8..c09b65369370c 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -342,6 +342,14 @@ pub mod pallet { WrongProposalLength, } + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + #[cfg(feautre = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { + Self::do_try_state(); + } + } + // Note that councillor operations are assigned to the operational class. #[pallet::call] impl, I: 'static> Pallet { @@ -969,6 +977,44 @@ impl, I: 'static> Pallet { }); 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 `ProposalsOf` storage map. + /// * `ProposalCount` must always equal to the number of proposals inside + /// the `Proposals` storage value. + /// + /// Looking at votes: + /// * The threshold of members required for a motion to pass cannot be + /// greater than the total number of members. + /// + /// Looking at members: + /// * The members count must never exceed `MaxMembers`. + #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] + fn do_try_state() -> Result<(), &'static str> { + Self::proposals().into_iter().for_each(|proposal| { + assert!(Self::proposal_of(proposal).is_some()); + }); + + assert_eq!(Self::proposals().into_iter().count(), Self::proposal_count() as usize); + + Self::proposals().into_iter().for_each(|proposal| { + if let Some(votes) = Self::voting(proposal) { + assert!(votes.threshold as usize <= Self::members().into_iter().count()); + } + }); + + assert!(Self::members().into_iter().count() <= T::MaxMembers::get() as usize); + + Ok(()) + } } impl, I: 'static> ChangeMembers for Pallet { From 115b1656eebb3af32f9d0ae3dacced5d441e54bf Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Sun, 19 Mar 2023 20:27:53 +0100 Subject: [PATCH 02/13] update tests --- frame/collective/src/lib.rs | 8 +-- frame/collective/src/tests.rs | 99 ++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 44 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index c09b65369370c..645f910969bce 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -988,8 +988,10 @@ impl, I: 'static> Pallet { /// /// * Each hash of a proposal that is stored inside `Proposals` must have a /// call mapped to it inside the `ProposalsOf` storage map. - /// * `ProposalCount` must always equal to the number of proposals inside - /// the `Proposals` storage value. + /// * `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. /// /// Looking at votes: /// * The threshold of members required for a motion to pass cannot be @@ -1003,7 +1005,7 @@ impl, I: 'static> Pallet { assert!(Self::proposal_of(proposal).is_some()); }); - assert_eq!(Self::proposals().into_iter().count(), Self::proposal_count() as usize); + assert!(Self::proposals().into_iter().count() <= Self::proposal_count() as usize); Self::proposals().into_iter().for_each(|proposal| { if let Some(votes) = Self::voting(proposal) { diff --git a/frame/collective/src/tests.rs b/frame/collective/src/tests.rs index 2d9b31a539d59..5dd5f9d1b4d42 100644 --- a/frame/collective/src/tests.rs +++ b/frame/collective/src/tests.rs @@ -156,23 +156,40 @@ impl Config for Test { type SetMembersOrigin = EnsureRoot; } -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 { @@ -187,7 +204,7 @@ fn record(event: RuntimeEvent) -> EventRecord { #[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::::new()); }); @@ -195,7 +212,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; @@ -263,7 +280,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, @@ -305,7 +322,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, @@ -335,7 +352,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; @@ -403,7 +420,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; @@ -473,7 +490,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; @@ -553,7 +570,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); @@ -601,7 +618,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); @@ -659,7 +676,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(); @@ -691,7 +708,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); @@ -718,7 +735,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, @@ -764,7 +781,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!( @@ -781,7 +798,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(); @@ -800,7 +817,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); @@ -820,7 +837,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(); @@ -889,7 +906,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(); @@ -947,7 +964,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; @@ -979,7 +996,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; @@ -1109,7 +1126,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; @@ -1168,7 +1185,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; @@ -1229,7 +1246,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; @@ -1290,7 +1307,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(); @@ -1322,7 +1339,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(); @@ -1381,7 +1398,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"; From 6eab1132f2698eafc40af56b988a6bc34522dd6b Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Mon, 20 Mar 2023 11:23:22 +0100 Subject: [PATCH 03/13] update check --- frame/collective/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 645f910969bce..dbc986229a2c2 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -994,12 +994,12 @@ impl, I: 'static> Pallet { /// count is not deducted. /// /// Looking at votes: - /// * The threshold of members required for a motion to pass cannot be - /// greater than the total number of members. + /// * The sum of aye and nay votes for a proposal can never exceed + /// `MaxMembers`. /// /// Looking at members: /// * The members count must never exceed `MaxMembers`. - #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] + #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), &'static str> { Self::proposals().into_iter().for_each(|proposal| { assert!(Self::proposal_of(proposal).is_some()); @@ -1009,7 +1009,9 @@ impl, I: 'static> Pallet { Self::proposals().into_iter().for_each(|proposal| { if let Some(votes) = Self::voting(proposal) { - assert!(votes.threshold as usize <= Self::members().into_iter().count()); + let ayes = votes.ayes.into_iter().count(); + let nays = votes.nays.into_iter().count(); + assert!(ayes.saturating_add(nays) <= T::MaxMembers::get() as usize); } }); From 574e8d4fc38ded6847ac60f9b9d77d4b10542c1b Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Mon, 20 Mar 2023 11:44:16 +0100 Subject: [PATCH 04/13] fix benchmarking --- frame/collective/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index d4b5af1bc1937..bcd203c3894a3 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -646,5 +646,5 @@ benchmarks_instance_pallet! { assert_last_event::(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); } From 431a40ef879d1ee3ca53a1c4c29b4fbe5a9ae032 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Mon, 20 Mar 2023 11:48:25 +0100 Subject: [PATCH 05/13] fix nonsense --- frame/collective/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index c39d8a29e1077..805eb2c1cec9e 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -1008,13 +1008,13 @@ impl, I: 'static> Pallet { Self::proposals().into_iter().for_each(|proposal| { if let Some(votes) = Self::voting(proposal) { - let ayes = votes.ayes.into_iter().count(); - let nays = votes.nays.into_iter().count(); + let ayes = votes.ayes.len(); + let nays = votes.nays.len(); assert!(ayes.saturating_add(nays) <= T::MaxMembers::get() as usize); } }); - assert!(Self::members().into_iter().count() <= T::MaxMembers::get() as usize); + assert!(Self::members().len() <= T::MaxMembers::get() as usize); Ok(()) } From 5bbcb36f497d88513f48cf7708e6b368e9a6a253 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Wed, 22 Mar 2023 20:23:28 +0100 Subject: [PATCH 06/13] Update frame/collective/src/lib.rs Co-authored-by: Oliver Tale-Yazdi --- frame/collective/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 805eb2c1cec9e..f3164f3b7e3d0 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -343,7 +343,7 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { - #[cfg(feautre = "try-runtime")] + #[cfg(feature = "try-runtime")] fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { Self::do_try_state(); } From e50e0de4a6d147211e92bac5370cb7ea1cd94cc2 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Wed, 22 Mar 2023 20:23:38 +0100 Subject: [PATCH 07/13] Update frame/collective/src/lib.rs Co-authored-by: Oliver Tale-Yazdi --- frame/collective/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index f3164f3b7e3d0..ed1e591234f5d 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -345,7 +345,7 @@ pub mod pallet { impl, I: 'static> Hooks> for Pallet { #[cfg(feature = "try-runtime")] fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { - Self::do_try_state(); + Self::do_try_state() } } From 38490dbe9875b7b6070f4d2be797dd01b77e572a Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Thu, 23 Mar 2023 11:20:00 +0100 Subject: [PATCH 08/13] unique proposal index --- frame/collective/src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index ed1e591234f5d..7daa6e71b6fb7 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -995,6 +995,7 @@ impl, I: 'static> Pallet { /// 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. /// /// Looking at members: /// * The members count must never exceed `MaxMembers`. @@ -1014,6 +1015,15 @@ impl, I: 'static> Pallet { } }); + Self::proposals().into_iter().for_each(|proposal| { + let mut proposal_indices = vec![]; + if let Some(votes) = Self::voting(proposal) { + let proposal_index = votes.index; + assert!(!proposal_indices.contains(&proposal_index)); + proposal_indices.push(proposal_index); + } + }); + assert!(Self::members().len() <= T::MaxMembers::get() as usize); Ok(()) From 846ccadc670fe3f12dcbac31339adbc58ecf445f Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Thu, 23 Mar 2023 11:24:55 +0100 Subject: [PATCH 09/13] prime must be a member of the collective --- frame/collective/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 7daa6e71b6fb7..953d7905cf942 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -999,6 +999,9 @@ impl, I: 'static> Pallet { /// /// Looking at members: /// * The members count must never exceed `MaxMembers`. + /// + /// Looking at prime account: + /// * The prime account must be a member of the collective. #[cfg(any(feature = "try-runtime", test))] fn do_try_state() -> Result<(), &'static str> { Self::proposals().into_iter().for_each(|proposal| { @@ -1026,6 +1029,10 @@ impl, I: 'static> Pallet { assert!(Self::members().len() <= T::MaxMembers::get() as usize); + if let Some(prime) = Self::prime() { + assert!(Self::members().contains(&prime)); + } + Ok(()) } } From 743a2aaa910e3b41269b0dcad99ef7c1730e9307 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Thu, 23 Mar 2023 11:39:01 +0100 Subject: [PATCH 10/13] oops --- frame/collective/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 953d7905cf942..4cbc7cbc6cba2 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -1018,8 +1018,8 @@ impl, I: 'static> Pallet { } }); + let mut proposal_indices = vec![]; Self::proposals().into_iter().for_each(|proposal| { - let mut proposal_indices = vec![]; if let Some(votes) = Self::voting(proposal) { let proposal_index = votes.index; assert!(!proposal_indices.contains(&proposal_index)); From 2ae6e9a7fd48781e317f561769cf60ae248e661c Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Mon, 27 Mar 2023 18:07:01 +0200 Subject: [PATCH 11/13] Add new checks --- frame/collective/src/lib.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 4cbc7cbc6cba2..eb836d38bf449 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -986,19 +986,22 @@ impl, I: 'static> Pallet { /// Looking at proposals: /// /// * Each hash of a proposal that is stored inside `Proposals` must have a - /// call mapped to it inside the `ProposalsOf` storage map. + /// 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. @@ -1009,6 +1012,7 @@ impl, I: 'static> Pallet { }); assert!(Self::proposals().into_iter().count() <= Self::proposal_count() as usize); + assert!(Self::proposals().into_iter().count() == >::iter_keys().count()); Self::proposals().into_iter().for_each(|proposal| { if let Some(votes) = Self::voting(proposal) { @@ -1027,8 +1031,14 @@ impl, I: 'static> Pallet { } }); + >::iter_keys().for_each(|proposal_hash| { + assert!(Self::proposals().contains(&proposal_hash)); + }); + assert!(Self::members().len() <= T::MaxMembers::get() as usize); + assert!(Self::members().windows(2).all(|members| members[0] <= members[1])); + if let Some(prime) = Self::prime() { assert!(Self::members().contains(&prime)); } From 87813e2cc047c9c2da9a24d84f65e9f3e6d88483 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Wed, 29 Mar 2023 17:31:50 +0200 Subject: [PATCH 12/13] use ensure --- frame/collective/src/lib.rs | 82 ++++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 23 deletions(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index eb836d38bf449..3e7c1f854b9d9 100644 --- a/frame/collective/src/lib.rs +++ b/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::{ @@ -1006,41 +1006,77 @@ impl, I: 'static> Pallet { /// Looking at prime account: /// * The prime account must be a member of the collective. #[cfg(any(feature = "try-runtime", test))] - fn do_try_state() -> Result<(), &'static str> { - Self::proposals().into_iter().for_each(|proposal| { - assert!(Self::proposal_of(proposal).is_some()); - }); - - assert!(Self::proposals().into_iter().count() <= Self::proposal_count() as usize); - assert!(Self::proposals().into_iter().count() == >::iter_keys().count()); - - Self::proposals().into_iter().for_each(|proposal| { + 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() == >::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(); - assert!(ayes.saturating_add(nays) <= T::MaxMembers::get() as usize); + + 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().for_each(|proposal| { + Self::proposals().into_iter().try_for_each(|proposal| -> DispatchResult { if let Some(votes) = Self::voting(proposal) { let proposal_index = votes.index; - assert!(!proposal_indices.contains(&proposal_index)); + ensure!( + !proposal_indices.contains(&proposal_index), + DispatchError::Other("The proposal index is not unique.") + ); proposal_indices.push(proposal_index); } - }); - - >::iter_keys().for_each(|proposal_hash| { - assert!(Self::proposals().contains(&proposal_hash)); - }); + Ok(()) + })?; + + >::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(()) + })?; - assert!(Self::members().len() <= T::MaxMembers::get() as usize); + ensure!( + Self::members().len() <= T::MaxMembers::get() as usize, + DispatchError::Other("The member count is greater than `MaxMembers`.") + ); - assert!(Self::members().windows(2).all(|members| members[0] <= members[1])); + 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() { - assert!(Self::members().contains(&prime)); + ensure!( + Self::members().contains(&prime), + DispatchError::Other("Prime account is not a member.") + ); } Ok(()) From 104914e812637b89d318c63a33201cb3fb1f6907 Mon Sep 17 00:00:00 2001 From: Sergej Sakac Date: Mon, 3 Apr 2023 19:43:10 +0200 Subject: [PATCH 13/13] fix --- frame/collective/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 3e7c1f854b9d9..5dadb234008c1 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -345,7 +345,8 @@ pub mod pallet { impl, I: 'static> Hooks> for Pallet { #[cfg(feature = "try-runtime")] fn try_state(_n: BlockNumberFor) -> Result<(), &'static str> { - Self::do_try_state() + Self::do_try_state()?; + Ok(()) } }