Collective weights (#5802)

substrate/Cargo.lock

@@ -3656,6 +3656,7 @@ dependencies = [
  "sp-std",
  "sp-transaction-pool",
  "sp-version",
+ "static_assertions",
  "substrate-wasm-builder-runner",
 ]
 

substrate/bin/node/runtime/Cargo.toml

@@ -17,6 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 codec = { package = "parity-scale-codec", version = "1.3.0", default-features = false, features = ["derive"] }
 integer-sqrt = { version = "0.1.2" }
 serde = { version = "1.0.102", optional = true }
+static_assertions = "1.1.0"
 
 # primitives
 sp-authority-discovery = { version = "2.0.0-dev", default-features = false, path = "../../../primitives/authority-discovery" }

substrate/bin/node/runtime/src/lib.rs

@@ -58,6 +58,8 @@ use pallet_transaction_payment_rpc_runtime_api::RuntimeDispatchInfo;
 use pallet_contracts_rpc_runtime_api::ContractExecResult;
 use pallet_session::{historical as pallet_session_historical};
 use sp_inherents::{InherentData, CheckInherentsResult};
+use codec::Encode;
+use static_assertions::const_assert;
 
 #[cfg(any(feature = "std", test))]
 pub use sp_runtime::BuildStorage;
@@ -67,7 +69,6 @@ pub use frame_system::Call as SystemCall;
 pub use pallet_contracts::Gas;
 pub use frame_support::StorageValue;
 pub use pallet_staking::StakerStatus;
-use codec::Encode;
 
 /// Implementations of some helper traits passed into runtime modules as associated types.
 pub mod impls;
@@ -90,7 +91,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 247,
+	spec_version: 248,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,
@@ -377,6 +378,7 @@ impl pallet_democracy::Trait for Runtime {
 
 parameter_types! {
 	pub const CouncilMotionDuration: BlockNumber = 5 * DAYS;
+	pub const CouncilMaxProposals: u32 = 100;
 }
 
 type CouncilCollective = pallet_collective::Instance1;
@@ -385,16 +387,20 @@ impl pallet_collective::Trait<CouncilCollective> for Runtime {
 	type Proposal = Call;
 	type Event = Event;
 	type MotionDuration = CouncilMotionDuration;
+	type MaxProposals = CouncilMaxProposals;
 }
 
+const DESIRED_MEMBERS: u32 = 13;
 parameter_types! {
 	pub const CandidacyBond: Balance = 10 * DOLLARS;
 	pub const VotingBond: Balance = 1 * DOLLARS;
 	pub const TermDuration: BlockNumber = 7 * DAYS;
-	pub const DesiredMembers: u32 = 13;
+	pub const DesiredMembers: u32 = DESIRED_MEMBERS;
 	pub const DesiredRunnersUp: u32 = 7;
 	pub const ElectionsPhragmenModuleId: LockIdentifier = *b"phrelect";
 }
+// Make sure that there are no more than `MAX_MEMBERS` members elected via phragmen.
+const_assert!(DESIRED_MEMBERS <= pallet_collective::MAX_MEMBERS);
 
 impl pallet_elections_phragmen::Trait for Runtime {
 	type ModuleId = ElectionsPhragmenModuleId;
@@ -417,6 +423,7 @@ impl pallet_elections_phragmen::Trait for Runtime {
 
 parameter_types! {
 	pub const TechnicalMotionDuration: BlockNumber = 5 * DAYS;
+	pub const TechnicalMaxProposals: u32 = 100;
 }
 
 type TechnicalCollective = pallet_collective::Instance2;
@@ -425,6 +432,7 @@ impl pallet_collective::Trait<TechnicalCollective> for Runtime {
 	type Proposal = Call;
 	type Event = Event;
 	type MotionDuration = TechnicalMotionDuration;
+	type MaxProposals = TechnicalMaxProposals;
 }
 
 impl pallet_membership::Trait<pallet_membership::Instance1> for Runtime {

substrate/frame/collective/src/benchmarking.rs

@@ -22,14 +22,14 @@ use frame_system::RawOrigin as SystemOrigin;
 use frame_system::EventRecord;
 use frame_benchmarking::{benchmarks_instance, account};
 use sp_runtime::traits::Bounded;
+use sp_std::mem::size_of;
 
+use frame_system::Call as SystemCall;
 use frame_system::Module as System;
 use crate::Module as Collective;
 
 const SEED: u32 = 0;
 
-const MAX_MEMBERS: u32 = 1000;
-const MAX_PROPOSALS: u32 = 100;
 const MAX_BYTES: u32 = 1_024;
 
 fn assert_last_event<T: Trait<I>, I: Instance>(generic_event: <T as Trait<I>>::Event) {
@@ -46,28 +46,64 @@ benchmarks_instance! {
 	set_members {
 		let m in 1 .. MAX_MEMBERS;
 		let n in 1 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 
 		// Set old members.
 		// We compute the difference of old and new members, so it should influence timing.
 		let mut old_members = vec![];
 		let mut last_old_member = T::AccountId::default();
-		for i in 0 .. n {
+		for i in 0 .. m {
 			last_old_member = account("old member", i, SEED);
 			old_members.push(last_old_member.clone());
 		}
+		let old_members_count = old_members.len() as u32;
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), old_members, Some(last_old_member))?;
+		Collective::<T, _>::set_members(
+			SystemOrigin::Root.into(),
+			old_members.clone(),
+			Some(last_old_member.clone()),
+			MAX_MEMBERS,
+		)?;
+
+		// Set a high threshold for proposals passing so that they stay around.
+		let threshold = m.max(2);
+		// Length of the proposals should be irrelevant to `set_members`.
+		let length = 100;
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; length]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(last_old_member.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				MAX_BYTES,
+			)?;
+			let hash = T::Hashing::hash_of(&proposal);
+			// Vote on the proposal to increase state relevant for `set_members`.
+			// Not voting for `last_old_member` because they proposed and not voting for the first member
+			// to keep the proposal from passing.
+			for j in 2 .. m - 1 {
+				let voter = &old_members[j as usize];
+				let approve = true;
+				Collective::<T, _>::vote(
+					SystemOrigin::Signed(voter.clone()).into(),
+					hash,
+					i,
+					approve,
+				)?;
+			}
+		}
 
 		// Construct `new_members`.
 		// It should influence timing since it will sort this vector.
 		let mut new_members = vec![];
 		let mut last_member = T::AccountId::default();
-		for i in 0 .. m {
+		for i in 0 .. n {
 			last_member = account("member", i, SEED);
 			new_members.push(last_member.clone());
 		}
 
-	}: _(SystemOrigin::Root, new_members.clone(), Some(last_member))
+	}: _(SystemOrigin::Root, new_members.clone(), Some(last_member), MAX_MEMBERS)
 	verify {
 		new_members.sort();
 		assert_eq!(Collective::<T, _>::members(), new_members);
@@ -77,9 +113,11 @@ benchmarks_instance! {
 		let m in 1 .. MAX_MEMBERS;
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
@@ -87,15 +125,17 @@ benchmarks_instance! {
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?;
 
-		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![1; b as usize]).into();
+		let proposal: T::Proposal = SystemCall::<T>::remark(vec![1; b as usize]).into();
 
-	}: _(SystemOrigin::Signed(caller), Box::new(proposal.clone()))
+	}: _(SystemOrigin::Signed(caller), Box::new(proposal.clone()), bytes_in_storage)
 	verify {
 		let proposal_hash = T::Hashing::hash_of(&proposal);
 		// Note that execution fails due to mis-matched origin
-		assert_last_event::<T, I>(RawEvent::MemberExecuted(proposal_hash, false).into());
+		assert_last_event::<T, I>(
+			RawEvent::MemberExecuted(proposal_hash, Err(DispatchError::BadOrigin)).into()
+		);
 	}
 
 	// This tests when execution would happen immediately after proposal
@@ -103,9 +143,11 @@ benchmarks_instance! {
 		let m in 1 .. MAX_MEMBERS;
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
@@ -113,169 +155,230 @@ benchmarks_instance! {
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?;
 
-		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![1; b as usize]).into();
+		let proposal: T::Proposal = SystemCall::<T>::remark(vec![1; b as usize]).into();
 		let threshold = 1;
 
-	}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()))
+	}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()), bytes_in_storage)
 	verify {
 		let proposal_hash = T::Hashing::hash_of(&proposal);
 		// Note that execution fails due to mis-matched origin
-		assert_last_event::<T, I>(RawEvent::Executed(proposal_hash, false).into());
+		assert_last_event::<T, I>(
+			RawEvent::Executed(proposal_hash, Err(DispatchError::BadOrigin)).into()
+		);
 	}
 
 	// This tests when proposal is created and queued as "proposed"
 	propose_proposed {
-		let m in 1 .. MAX_MEMBERS;
-		let p in 0 .. MAX_PROPOSALS;
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
-
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
-
-		let threshold = m.max(2);
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?;
 
+		let threshold = m;
 		// Add previous proposals.
+		for i in 0 .. p - 1 {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(caller.clone()).into(),
+				threshold,
+				Box::new(proposal),
+				bytes_in_storage,
+			)?;
+		}
+
+		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
+
+		let proposal: T::Proposal = SystemCall::<T>::remark(vec![p as u8; b as usize]).into();
+
+	}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()), bytes_in_storage)
+	verify {
+		// New proposal is recorded
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+		let proposal_hash = T::Hashing::hash_of(&proposal);
+		assert_last_event::<T, I>(RawEvent::Proposed(caller, p - 1, proposal_hash, threshold).into());
+	}
+
+	vote {
+		// We choose 5 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
+		let m in 5 .. MAX_MEMBERS;
+
+		let p = T::MaxProposals::get();
+		let b = MAX_BYTES;
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
+		// Construct `members`.
+		let mut members = vec![];
+		let proposer: T::AccountId = account("proposer", 0, SEED);
+		members.push(proposer.clone());
+		for i in 1 .. m - 1 {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
+		let voter: T::AccountId = account("voter", 0, SEED);
+		members.push(voter.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None, MAX_MEMBERS)?;
+
+		// Threshold is 1 less than the number of members so that one person can vote nay
+		let threshold = m - 1;
+
+		// Add previous proposals
+		let mut last_hash = T::Hash::default();
 		for i in 0 .. p {
 			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal))?;
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(proposer.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				bytes_in_storage,
+			)?;
+			last_hash = T::Hashing::hash_of(&proposal);
 		}
 
+		let index = p - 1;
+		// Have almost everyone vote aye on last proposal, while keeping it from passing.
+		// Proposer already voted aye so we start at 1.
+		for j in 1 .. m - 3 {
+			let voter = &members[j as usize];
+			let approve = true;
+			Collective::<T, _>::vote(
+				SystemOrigin::Signed(voter.clone()).into(),
+				last_hash.clone(),
+				index,
+				approve,
+			)?;
+		}
+		// Voter votes aye without resolving the vote.
+		let approve = true;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(voter.clone()).into(),
+			last_hash.clone(),
+			index,
+			approve,
+		)?;
+
 		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
-		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![p as u8; b as usize]).into();
+		// Voter switches vote to nay, but does not kill the vote, just updates + inserts
+		let approve = false;
 
-	}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()))
+	}: _(SystemOrigin::Signed(voter), last_hash.clone(), index, approve)
 	verify {
-		// New proposal is recorded
-		assert_eq!(Collective::<T, _>::proposals().len(), (p + 1) as usize);
-		let proposal_hash = T::Hashing::hash_of(&proposal);
-		assert_last_event::<T, I>(RawEvent::Proposed(caller, p, proposal_hash, threshold).into());
+		// All proposals exist and the last proposal has just been updated.
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+		let voting = Collective::<T, _>::voting(&last_hash).ok_or(Error::<T, I>::ProposalMissing)?;
+		assert_eq!(voting.ayes.len(), (m - 3) as usize);
+		assert_eq!(voting.nays.len(), 1);
 	}
 
-	vote_insert {
-		let m in 2 .. MAX_MEMBERS;
-		let p in 1 .. MAX_PROPOSALS;
+	close_early_disapproved {
+		// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
+		let m in 4 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		let proposer: T::AccountId = account("proposer", 0, SEED);
+		members.push(proposer.clone());
+		for i in 1 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
+		let voter: T::AccountId = account("voter", 0, SEED);
+		members.push(voter.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None, MAX_MEMBERS)?;
 
-		let caller: T::AccountId = account("caller", 0, SEED);
-		members.push(caller.clone());
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
-
-		// Threshold is 1 less than the number of members so that one person can vote nay
+		// Threshold is total members so that one nay will disapprove the vote
 		let threshold = m;
 
 		// Add previous proposals
 		let mut last_hash = T::Hash::default();
 		for i in 0 .. p {
 			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(proposer.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				bytes_in_storage,
+			)?;
 			last_hash = T::Hashing::hash_of(&proposal);
 		}
 
-		// Have everyone vote aye on last proposal, while keeping it from passing
-		for j in 2 .. m {
+		let index = p - 1;
+		// Have most everyone vote aye on last proposal, while keeping it from passing.
+		// Proposer already voted aye so we start at 1.
+		for j in 1 .. m - 2 {
 			let voter = &members[j as usize];
 			let approve = true;
-			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+			Collective::<T, _>::vote(
+				SystemOrigin::Signed(voter.clone()).into(),
+				last_hash.clone(),
+				index,
+				approve,
+			)?;
 		}
+		// Voter votes aye without resolving the vote.
+		let approve = true;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(voter.clone()).into(),
+			last_hash.clone(),
+			index,
+			approve,
+		)?;
 
 		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
-		// Caller switches vote to nay, but does not kill the vote, just updates + inserts
-		let index = p - 1;
+		// Voter switches vote to nay, which kills the vote
 		let approve = false;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(voter.clone()).into(),
+			last_hash.clone(),
+			index,
+			approve,
+		)?;
 
-	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
-	verify {
-		// All proposals exist and the last proposal has just been updated.
-		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
-		let voting = Collective::<T, _>::voting(&last_hash).ok_or(Error::<T, I>::ProposalMissing)?;
-		assert_eq!(voting.ayes.len(), (m - 2) as usize);
-		assert_eq!(voting.nays.len(), 1);
-	}
-
-	vote_disapproved {
-		let m in 2 .. MAX_MEMBERS;
-		let p in 1 .. MAX_PROPOSALS;
-		let b in 1 .. MAX_BYTES;
-
-		// Construct `members`.
-		let mut members = vec![];
-		for i in 0 .. m {
-			let member = account("member", i, SEED);
-			members.push(member);
-		}
-
-		let caller: T::AccountId = account("caller", 0, SEED);
-		members.push(caller.clone());
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
-
-		// Threshold is total members so that one nay will disapprove the vote
-		let threshold = m + 1;
-
-		// Add previous proposals
-		let mut last_hash = T::Hash::default();
-		for i in 0 .. p {
-			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
-			last_hash = T::Hashing::hash_of(&proposal);
-		}
-
-		// Have everyone vote aye on last proposal, while keeping it from passing
-		for j in 1 .. m {
-			let voter = &members[j as usize];
-			let approve = true;
-			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
-		}
-
-		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
-
-		// Caller switches vote to nay, which kills the vote
-		let index = p - 1;
-		let approve = false;
-
-	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
+	}: close(SystemOrigin::Signed(voter), last_hash.clone(), index, Weight::max_value(), bytes_in_storage)
 	verify {
 		// The last proposal is removed.
 		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
 		assert_last_event::<T, I>(RawEvent::Disapproved(last_hash).into());
 	}
 
-	vote_approved {
-		let m in 2 .. MAX_MEMBERS;
-		let p in 1 .. MAX_PROPOSALS;
+	close_early_approved {
+		// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
+		let m in 4 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
-
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None, MAX_MEMBERS)?;
 
 		// Threshold is 2 so any two ayes will approve the vote
 		let threshold = 2;
@@ -284,102 +387,156 @@ benchmarks_instance! {
 		let mut last_hash = T::Hash::default();
 		for i in 0 .. p {
 			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(caller.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				bytes_in_storage,
+			)?;
 			last_hash = T::Hashing::hash_of(&proposal);
 		}
 
 		// Caller switches vote to nay on their own proposal, allowing them to be the deciding approval vote
-		Collective::<T, _>::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(caller.clone()).into(),
+			last_hash.clone(),
+			p - 1,
+			false,
+		)?;
 
-		// Have everyone vote nay on last proposal, while keeping it from failing
-		for j in 2 .. m {
+		// Have almost everyone vote nay on last proposal, while keeping it from failing.
+		for j in 2 .. m - 1 {
 			let voter = &members[j as usize];
 			let approve = false;
-			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+			Collective::<T, _>::vote(
+				SystemOrigin::Signed(voter.clone()).into(),
+				last_hash.clone(),
+				p - 1,
+				approve,
+			)?;
 		}
 
 		// Member zero is the first aye
-		Collective::<T, _>::vote(SystemOrigin::Signed(members[0].clone()).into(), last_hash.clone(), p - 1, true)?;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(members[0].clone()).into(),
+			last_hash.clone(),
+			p - 1,
+			true,
+		)?;
 
 		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
 		// Caller switches vote to aye, which passes the vote
 		let index = p - 1;
 		let approve = true;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(caller.clone()).into(),
+			last_hash.clone(),
+			index, approve,
+		)?;
 
-	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
+	}: close(SystemOrigin::Signed(caller), last_hash.clone(), index, Weight::max_value(), bytes_in_storage)
 	verify {
 		// The last proposal is removed.
 		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
-		assert_last_event::<T, I>(RawEvent::Executed(last_hash, false).into());
+		assert_last_event::<T, I>(RawEvent::Executed(last_hash, Err(DispatchError::BadOrigin)).into());
 	}
 
 	close_disapproved {
-		let m in 2 .. MAX_MEMBERS;
-		let p in 1 .. MAX_PROPOSALS;
+		// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
+		let m in 4 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
-
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?;
+		Collective::<T, _>::set_members(
+			SystemOrigin::Root.into(),
+			members.clone(),
+			Some(caller.clone()),
+			MAX_MEMBERS,
+		)?;
 
 		// Threshold is one less than total members so that two nays will disapprove the vote
-		let threshold = m;
+		let threshold = m - 1;
 
 		// Add proposals
 		let mut last_hash = T::Hash::default();
 		for i in 0 .. p {
 			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(caller.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				bytes_in_storage,
+			)?;
 			last_hash = T::Hashing::hash_of(&proposal);
 		}
 
-		// Have everyone vote aye on last proposal, while keeping it from passing
-		// A few abstainers will be the nay votes needed to fail the vote
-		for j in 2 .. m {
+		let index = p - 1;
+		// Have almost everyone vote aye on last proposal, while keeping it from passing.
+		// A few abstainers will be the nay votes needed to fail the vote.
+		for j in 2 .. m - 1 {
 			let voter = &members[j as usize];
 			let approve = true;
-			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+			Collective::<T, _>::vote(
+				SystemOrigin::Signed(voter.clone()).into(),
+				last_hash.clone(),
+				index,
+				approve,
+			)?;
 		}
 
 		// caller is prime, prime votes nay
-		Collective::<T, _>::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?;
+		Collective::<T, _>::vote(
+			SystemOrigin::Signed(caller.clone()).into(),
+			last_hash.clone(),
+			index,
+			false,
+		)?;
 
 		System::<T>::set_block_number(T::BlockNumber::max_value());
 		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
 		// Prime nay will close it as disapproved
-	}: close(SystemOrigin::Signed(caller), last_hash, p - 1)
+	}: close(SystemOrigin::Signed(caller), last_hash, index, Weight::max_value(), bytes_in_storage)
 	verify {
 		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
 		assert_last_event::<T, I>(RawEvent::Disapproved(last_hash).into());
 	}
 
-
 	close_approved {
-		let m in 2 .. MAX_MEMBERS;
-		let p in 1 .. MAX_PROPOSALS;
+		// We choose 4 as a minimum so we always trigger a vote in the voting loop (`for j in ...`)
+		let m in 4 .. MAX_MEMBERS;
+		let p in 1 .. T::MaxProposals::get();
 		let b in 1 .. MAX_BYTES;
 
+		let bytes_in_storage = b + size_of::<u32>() as u32;
+
 		// Construct `members`.
 		let mut members = vec![];
-		for i in 0 .. m {
+		for i in 0 .. m - 1 {
 			let member = account("member", i, SEED);
 			members.push(member);
 		}
 		let caller: T::AccountId = account("caller", 0, SEED);
 		members.push(caller.clone());
-
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?;
+		Collective::<T, _>::set_members(
+			SystemOrigin::Root.into(),
+			members.clone(),
+			Some(caller.clone()),
+			MAX_MEMBERS,
+		)?;
 
 		// Threshold is two, so any two ayes will pass the vote
 		let threshold = 2;
@@ -388,17 +545,27 @@ benchmarks_instance! {
 		let mut last_hash = T::Hash::default();
 		for i in 0 .. p {
 			// Proposals should be different so that different proposal hashes are generated
-			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			let proposal: T::Proposal = SystemCall::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(
+				SystemOrigin::Signed(caller.clone()).into(),
+				threshold,
+				Box::new(proposal.clone()),
+				bytes_in_storage,
+			)?;
 			last_hash = T::Hashing::hash_of(&proposal);
 		}
 
-		// Have everyone vote nay on last proposal, while keeping it from failing
-		// A few abstainers will be the aye votes needed to pass the vote
-		for j in 2 .. m {
+		// Have almost everyone vote nay on last proposal, while keeping it from failing.
+		// A few abstainers will be the aye votes needed to pass the vote.
+		for j in 2 .. m - 1 {
 			let voter = &members[j as usize];
 			let approve = false;
-			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+			Collective::<T, _>::vote(
+				SystemOrigin::Signed(voter.clone()).into(),
+				last_hash.clone(),
+				p - 1,
+				approve
+			)?;
 		}
 
 		// caller is prime, prime already votes aye by creating the proposal
@@ -406,10 +573,10 @@ benchmarks_instance! {
 		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
 		// Prime aye will close it as approved
-	}: close(SystemOrigin::Signed(caller), last_hash, p - 1)
+	}: close(SystemOrigin::Signed(caller), last_hash, p - 1, Weight::max_value(), bytes_in_storage)
 	verify {
 		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
-		assert_last_event::<T, I>(RawEvent::Executed(last_hash, false).into());
+		assert_last_event::<T, I>(RawEvent::Executed(last_hash, Err(DispatchError::BadOrigin)).into());
 	}
 }
 
@@ -420,16 +587,64 @@ mod tests {
 	use frame_support::assert_ok;
 
 	#[test]
-	fn test_benchmarks() {
+	fn set_members() {
 		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_set_members::<Test>());
+		});
+	}
+
+	#[test]
+	fn execute() {
+		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_execute::<Test>());
+		});
+	}
+
+	#[test]
+	fn propose_execute() {
+		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_propose_execute::<Test>());
+		});
+	}
+
+	#[test]
+	fn propose_proposed() {
+		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_propose_proposed::<Test>());
-			assert_ok!(test_benchmark_vote_insert::<Test>());
-			assert_ok!(test_benchmark_vote_disapproved::<Test>());
-			assert_ok!(test_benchmark_vote_approved::<Test>());
+		});
+	}
+
+	#[test]
+	fn vote() {
+		new_test_ext().execute_with(|| {
+			assert_ok!(test_benchmark_vote::<Test>());
+		});
+	}
+
+	#[test]
+	fn close_early_disapproved() {
+		new_test_ext().execute_with(|| {
+			assert_ok!(test_benchmark_close_early_disapproved::<Test>());
+		});
+	}
+
+	#[test]
+	fn close_early_approved() {
+		new_test_ext().execute_with(|| {
+			assert_ok!(test_benchmark_close_early_approved::<Test>());
+		});
+	}
+
+	#[test]
+	fn close_disapproved() {
+		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_close_disapproved::<Test>());
+		});
+	}
+
+	#[test]
+	fn close_approved() {
+		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_close_approved::<Test>());
 		});
 	}