diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 117497eb32..0060543e01 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -3656,6 +3656,7 @@ dependencies = [ "sp-std", "sp-transaction-pool", "sp-version", + "static_assertions", "substrate-wasm-builder-runner", ] diff --git a/substrate/bin/node/runtime/Cargo.toml b/substrate/bin/node/runtime/Cargo.toml index 00d1c6dd72..bae21fd6c8 100644 --- a/substrate/bin/node/runtime/Cargo.toml +++ b/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" } diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 64b4cc0e64..1eec930dfb 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/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 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 for Runtime { type Proposal = Call; type Event = Event; type MotionDuration = TechnicalMotionDuration; + type MaxProposals = TechnicalMaxProposals; } impl pallet_membership::Trait for Runtime { diff --git a/substrate/frame/collective/src/benchmarking.rs b/substrate/frame/collective/src/benchmarking.rs index 5c25051fd0..77473abef4 100644 --- a/substrate/frame/collective/src/benchmarking.rs +++ b/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, I: Instance>(generic_event: >::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::::set_members(SystemOrigin::Root.into(), old_members, Some(last_old_member))?; + Collective::::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::::remark(vec![i as u8; length]).into(); + Collective::::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::::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::::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::() 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::::set_members(SystemOrigin::Root.into(), members, None)?; + Collective::::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?; - let proposal: T::Proposal = frame_system::Call::::remark(vec![1; b as usize]).into(); + let proposal: T::Proposal = SystemCall::::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::(RawEvent::MemberExecuted(proposal_hash, false).into()); + assert_last_event::( + 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::() 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::::set_members(SystemOrigin::Root.into(), members, None)?; + Collective::::set_members(SystemOrigin::Root.into(), members, None, MAX_MEMBERS)?; - let proposal: T::Proposal = frame_system::Call::::remark(vec![1; b as usize]).into(); + let proposal: T::Proposal = SystemCall::::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::(RawEvent::Executed(proposal_hash, false).into()); + assert_last_event::( + 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::() 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::::set_members(SystemOrigin::Root.into(), members, None)?; - - let threshold = m.max(2); + Collective::::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::::remark(vec![i as u8; b as usize]).into(); + Collective::::propose( + SystemOrigin::Signed(caller.clone()).into(), + threshold, + Box::new(proposal), + bytes_in_storage, + )?; + } + + assert_eq!(Collective::::proposals().len(), (p - 1) as usize); + + let proposal: T::Proposal = SystemCall::::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::::proposals().len(), p as usize); + let proposal_hash = T::Hashing::hash_of(&proposal); + assert_last_event::(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::() 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::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal))?; + let proposal: T::Proposal = SystemCall::::remark(vec![i as u8; b as usize]).into(); + Collective::::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::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + index, + approve, + )?; + } + // Voter votes aye without resolving the vote. + let approve = true; + Collective::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + index, + approve, + )?; + assert_eq!(Collective::::proposals().len(), p as usize); - let proposal: T::Proposal = frame_system::Call::::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::::proposals().len(), (p + 1) as usize); - let proposal_hash = T::Hashing::hash_of(&proposal); - assert_last_event::(RawEvent::Proposed(caller, p, proposal_hash, threshold).into()); + // All proposals exist and the last proposal has just been updated. + assert_eq!(Collective::::proposals().len(), p as usize); + let voting = Collective::::voting(&last_hash).ok_or(Error::::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::() 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::::set_members(SystemOrigin::Root.into(), members.clone(), None, MAX_MEMBERS)?; - let caller: T::AccountId = account("caller", 0, SEED); - members.push(caller.clone()); - Collective::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?; + let proposal: T::Proposal = SystemCall::::remark(vec![i as u8; b as usize]).into(); + Collective::::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::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; + Collective::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + index, + approve, + )?; } + // Voter votes aye without resolving the vote. + let approve = true; + Collective::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + index, + approve, + )?; assert_eq!(Collective::::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::::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::::proposals().len(), p as usize); - let voting = Collective::::voting(&last_hash).ok_or(Error::::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::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::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::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; - } - - assert_eq!(Collective::::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::::proposals().len(), (p - 1) as usize); assert_last_event::(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::() 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::::set_members(SystemOrigin::Root.into(), members.clone(), None)?; + Collective::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?; + let proposal: T::Proposal = SystemCall::::remark(vec![i as u8; b as usize]).into(); + Collective::::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::::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?; + Collective::::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::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; + Collective::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + p - 1, + approve, + )?; } // Member zero is the first aye - Collective::::vote(SystemOrigin::Signed(members[0].clone()).into(), last_hash.clone(), p - 1, true)?; + Collective::::vote( + SystemOrigin::Signed(members[0].clone()).into(), + last_hash.clone(), + p - 1, + true, + )?; assert_eq!(Collective::::proposals().len(), p as usize); // Caller switches vote to aye, which passes the vote let index = p - 1; let approve = true; + Collective::::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::::proposals().len(), (p - 1) as usize); - assert_last_event::(RawEvent::Executed(last_hash, false).into()); + assert_last_event::(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::() 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::::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?; + Collective::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?; + let proposal: T::Proposal = SystemCall::::remark(vec![i as u8; b as usize]).into(); + Collective::::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::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; + Collective::::vote( + SystemOrigin::Signed(voter.clone()).into(), + last_hash.clone(), + index, + approve, + )?; } // caller is prime, prime votes nay - Collective::::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?; + Collective::::vote( + SystemOrigin::Signed(caller.clone()).into(), + last_hash.clone(), + index, + false, + )?; System::::set_block_number(T::BlockNumber::max_value()); assert_eq!(Collective::::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::::proposals().len(), (p - 1) as usize); assert_last_event::(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::() 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::::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?; + Collective::::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::::remark(vec![i as u8; b as usize]).into(); - Collective::::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?; + let proposal: T::Proposal = SystemCall::::remark(vec![i as u8; b as usize]).into(); + Collective::::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::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; + Collective::::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::::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::::proposals().len(), (p - 1) as usize); - assert_last_event::(RawEvent::Executed(last_hash, false).into()); + assert_last_event::(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] + fn execute() { + new_test_ext().execute_with(|| { assert_ok!(test_benchmark_execute::()); + }); + } + + #[test] + fn propose_execute() { + new_test_ext().execute_with(|| { assert_ok!(test_benchmark_propose_execute::()); + }); + } + + #[test] + fn propose_proposed() { + new_test_ext().execute_with(|| { assert_ok!(test_benchmark_propose_proposed::()); - assert_ok!(test_benchmark_vote_insert::()); - assert_ok!(test_benchmark_vote_disapproved::()); - assert_ok!(test_benchmark_vote_approved::()); + }); + } + + #[test] + fn vote() { + new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_vote::()); + }); + } + + #[test] + fn close_early_disapproved() { + new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_close_early_disapproved::()); + }); + } + + #[test] + fn close_early_approved() { + new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_close_early_approved::()); + }); + } + + #[test] + fn close_disapproved() { + new_test_ext().execute_with(|| { assert_ok!(test_benchmark_close_disapproved::()); + }); + } + + #[test] + fn close_approved() { + new_test_ext().execute_with(|| { assert_ok!(test_benchmark_close_approved::()); }); } diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index 662465616e..31c93bf71d 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -19,6 +19,8 @@ //! //! The membership can be provided in one of two ways: either directly, using the Root-dispatchable //! function `set_members`, or indirectly, through implementing the `ChangeMembers`. +//! The pallet assumes that the amount of members stays at or below `MAX_MEMBERS` for its weight +//! calculations, but enforces this neither in `set_members` nor in `change_members_sorted`. //! //! A "prime" member may be set allowing their vote to act as the default vote in case of any //! abstentions after the voting period. @@ -38,13 +40,19 @@ use sp_std::{prelude::*, result}; use sp_core::u32_trait::Value as U32; -use sp_runtime::RuntimeDebug; -use sp_runtime::traits::Hash; +use sp_io::storage; +use sp_runtime::{RuntimeDebug, traits::Hash}; + use frame_support::{ - dispatch::{Dispatchable, Parameter}, codec::{Encode, Decode}, - traits::{Get, ChangeMembers, InitializeMembers, EnsureOrigin}, decl_module, decl_event, - decl_storage, decl_error, ensure, - weights::DispatchClass, + codec::{Decode, Encode}, + debug, decl_error, decl_event, decl_module, decl_storage, + dispatch::{ + DispatchError, DispatchResult, DispatchResultWithPostInfo, Dispatchable, Parameter, + PostDispatchInfo, + }, + ensure, + traits::{ChangeMembers, EnsureOrigin, Get, InitializeMembers}, + weights::{DispatchClass, GetDispatchInfo, Weight}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -60,18 +68,31 @@ pub type ProposalIndex = u32; /// vote exactly once, therefore also the number of votes for any given motion. pub type MemberCount = u32; +/// The maximum number of members supported by the pallet. Used for weight estimation. +/// +/// NOTE: +/// + Benchmarks will need to be re-run and weights adjusted if this changes. +/// + This pallet assumes that dependents keep to the limit without enforcing it. +pub const MAX_MEMBERS: MemberCount = 100; + pub trait Trait: frame_system::Trait { /// The outer origin type. type Origin: From>; /// The outer call dispatch type. - type Proposal: Parameter + Dispatchable>::Origin> + From>; + type Proposal: Parameter + + Dispatchable>::Origin, PostInfo=PostDispatchInfo> + + From> + + GetDispatchInfo; /// The outer event type. type Event: From> + Into<::Event>; /// The time-out for council motions. type MotionDuration: Get; + + /// Maximum number of proposals allowed to be active in parallel. + type MaxProposals: Get; } /// Origin for the collective module. @@ -144,9 +165,9 @@ decl_event! { /// A motion was not approved by the required threshold. Disapproved(Hash), /// A motion was executed; `bool` is true if returned without error. - Executed(Hash, bool), + Executed(Hash, DispatchResult), /// A single member did some action; `bool` is true if returned without error. - MemberExecuted(Hash, bool), + MemberExecuted(Hash, DispatchResult), /// A proposal was closed after its duration was up. Closed(Hash, MemberCount, MemberCount), } @@ -168,12 +189,152 @@ decl_error! { AlreadyInitialized, /// The close call is made too early, before the end of the voting. TooEarly, + /// There can only be a maximum of `MaxProposals` active proposals. + TooManyProposals, + /// The given weight bound for the proposal was too low. + WrongProposalWeight, + /// The given length bound for the proposal was too low. + WrongProposalLength, } } -// Note: this module is not benchmarked. The weights are obtained based on the similarity of the -// executed logic with other democracy function. Note that councillor operations are assigned to the -// operational class. +/// Functions for calcuating the weight of dispatchables. +mod weight_for { + use frame_support::{traits::Get, weights::Weight}; + use super::{Trait, Instance}; + + /// Calculate the weight for `set_members`. + /// + /// Based on benchmark: + /// 0 + M * 20.47 + N * 0.109 + P * 26.29 µs (min squares analysis) + /// + /// Note: The complexity of `set_members` is quadratic (`O(MP + N)`), so the linear approximation + /// of the benchmark is not always permissible. It is here, though, because the linear approximation + /// covered the range of possible values and we estimate weight via the worst case (max paramter + /// values) before execution so we can be sure that we are only overestimating. + pub(crate) fn set_members, I: Instance>( + old_count: Weight, + new_count: Weight, + proposals: Weight, + ) -> Weight { + let db = T::DbWeight::get(); + db.reads_writes(1, 1) // mutate `Members` + .saturating_add(db.writes(1)) // set `Prime` + .saturating_add(db.reads(1)) // read `Proposals` + .saturating_add(db.reads_writes(proposals, proposals)) // update votes (`Voting`) + .saturating_add(old_count.saturating_mul(21_000_000)) // M + .saturating_add(new_count.saturating_mul(110_000)) // N + .saturating_add(proposals.saturating_mul(27_000_000)) // P + } + + /// Calculate the weight for `execute`. + /// + /// Based on benchmark: + /// 22.62 + M * 0.115 + B * 0.003 µs (min squares analysis) + pub(crate) fn execute, I: Instance>( + members: Weight, + proposal: Weight, + length: Weight, + ) -> Weight { + T::DbWeight::get().reads(1) // read members for `is_member` + .saturating_add(23_000_000) // constant + .saturating_add(length.saturating_mul(4_000)) // B + .saturating_add(members.saturating_mul(120_000)) // M + .saturating_add(proposal) // P + } + + /// Calculate the weight for `propose` if the proposal is executed straight away (`threshold < 2`). + /// + /// Based on benchmark: + /// 28.12 + M * 0.218 + B * 0.003 µs (min squares analysis) + pub(crate) fn propose_execute, I: Instance>( + members: Weight, + proposal: Weight, + length: Weight, + ) -> Weight { + T::DbWeight::get().reads(2) // `is_member` + `contains_key` + .saturating_add(29_000_000) // constant + .saturating_add(length.saturating_mul(3_000)) // B + .saturating_add(members.saturating_mul(220_000)) // M + .saturating_add(proposal) // P1 + } + + /// Calculate the weight for `propose` if the proposal is put up for a vote (`threshold >= 2`). + /// + /// Based on benchmark: + /// 49.75 + M * 0.105 + P2 0.502 + B * 0.006 µs (min squares analysis) + pub(crate) fn propose_proposed, I: Instance>( + members: Weight, + proposals: Weight, + length: Weight, + ) -> Weight { + T::DbWeight::get().reads(2) // `is_member` + `contains_key` + .saturating_add(T::DbWeight::get().reads_writes(2, 4)) // `proposal` insertion + .saturating_add(50_000_000) // constant + .saturating_add(length.saturating_mul(6_000)) // B + .saturating_add(members.saturating_mul(110_000)) // M + .saturating_add(proposals.saturating_mul(510_000)) // P2 + } + + /// Calculate the weight for `vote`. + /// + /// Based on benchmark: + /// 24.03 + M * 0.349 + P * 0.119 + B * 0.003 µs (min squares analysis) + pub(crate) fn vote, I: Instance>( + members: Weight, + ) -> Weight { + T::DbWeight::get().reads(1) // read `Members` + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) // mutate `Voting` + .saturating_add(30_000_000) // constant + .saturating_add(members.saturating_mul(500_000)) // M + } + + /// Calculate the weight for `close`. + /// + /// Based on benchmarks: + /// - early disapproved: 37.21 + M * 0.239 + P2 * 0.466 + B * 0.002 µs (min squares analysis) + /// - early approved: 50.82 + M * 0.211 + P2 * 0.478 + B * 0.008 µs (min squares analysis) + /// - disapproved: 51.08 + M * 0.224 + P2 * 0.475 + B * 0.003 µs (min squares analysis) + /// - approved: 65.95 + M * 0.226 + P2 * 0.487 + B * 0.005 µs (min squares analysis) + pub(crate) fn close, I: Instance>( + members: Weight, + proposal_weight: Weight, + proposals: Weight, + length: Weight, + ) -> Weight { + let db = T::DbWeight::get(); + close_without_finalize::(members, length) + .saturating_add(db.reads(1)) // `Prime` + .saturating_add(db.writes(1)) // `Proposals` + .saturating_add(db.writes(1)) // `Voting` + .saturating_add(proposal_weight) // P1 + .saturating_add(proposals.saturating_mul(490_000)) // P2 + } + + /// Calculate the weight for `close` without the call to `finalize_proposal`. + pub(crate) fn close_without_finalize, I: Instance>( + members: Weight, + length: Weight, + ) -> Weight { + T::DbWeight::get().reads(3) // `Members`, `Voting`, `ProposalOf` + .saturating_add(66_000_000) // constant + .saturating_add(length.saturating_mul(8_000)) // B + .saturating_add(members.saturating_mul(250_000)) // M + } +} + +/// Return the weight of a dispatch call result as an `Option`. +/// +/// Will return the weight regardless of what the state of the result is. +fn get_result_weight(result: DispatchResultWithPostInfo) -> Option { + match result { + Ok(post_info) => post_info.actual_weight, + Err(err) => err.post_info.actual_weight, + } +} + + +// Note that councillor operations are assigned to the operational class. decl_module! { pub struct Module, I: Instance=DefaultInstance> for enum Call where origin: ::Origin { type Error = Error; @@ -182,72 +343,231 @@ decl_module! { /// Set the collective's membership. /// - /// - `new_members`: The new member list. Be nice to the chain and - // provide it sorted. + /// - `new_members`: The new member list. Be nice to the chain and provide it sorted. /// - `prime`: The prime member whose vote sets the default. + /// - `old_count`: The upper bound for the previous number of members in storage. + /// Used for weight estimation. /// /// Requires root origin. - #[weight = (100_000_000, DispatchClass::Operational)] - fn set_members(origin, new_members: Vec, prime: Option) { + /// + /// NOTE: Does not enforce the expected `MAX_MEMBERS` limit on the amount of members, but + /// the weight estimations rely on it to estimate dispatchable weight. + /// + /// # + /// ## Weight + /// - `O(MP + N)` where: + /// - `M` old-members-count (code- and governance-bounded) + /// - `N` new-members-count (code- and governance-bounded) + /// - `P` proposals-count (code-bounded) + /// - DB: + /// - 1 storage mutation (codec `O(M)` read, `O(N)` write) for reading and writing the members + /// - 1 storage read (codec `O(P)`) for reading the proposals + /// - `P` storage mutations (codec `O(M)`) for updating the votes for each proposal + /// - 1 storage write (codec `O(1)`) for deleting the old `prime` and setting the new one + /// # + #[weight = ( + weight_for::set_members::( + (*old_count).into(), // M + new_members.len() as Weight, // N + T::MaxProposals::get().into(), // P + ), + DispatchClass::Operational + )] + fn set_members(origin, + new_members: Vec, + prime: Option, + old_count: MemberCount, + ) -> DispatchResultWithPostInfo { ensure_root(origin)?; + if new_members.len() > MAX_MEMBERS as usize { + debug::error!( + "New members count exceeds maximum amount of members expected. (expected: {}, actual: {})", + MAX_MEMBERS, + new_members.len() + ); + } + + let old = Members::::get(); + if old.len() > old_count as usize { + debug::warn!( + "Wrong count used to estimate set_members weight. (expected: {}, actual: {})", + old_count, + old.len() + ); + } let mut new_members = new_members; new_members.sort(); - let old = Members::::get(); - >::set_members_sorted(&new_members[..], &old); + >::set_members_sorted(&new_members, &old); Prime::::set(prime); + + Ok(Some(weight_for::set_members::( + old.len() as Weight, // M + new_members.len() as Weight, // N + T::MaxProposals::get().into(), // P + )).into()) } /// Dispatch a proposal from a member using the `Member` origin. /// /// Origin must be a member of the collective. - #[weight = (100_000_000, DispatchClass::Operational)] - fn execute(origin, proposal: Box<>::Proposal>) { + /// + /// # + /// ## Weight + /// - `O(M + P)` where `M` members-count (code-bounded) and `P` complexity of dispatching `proposal` + /// - DB: 1 read (codec `O(M)`) + DB access of `proposal` + /// - 1 event + /// # + #[weight = ( + weight_for::execute::( + MAX_MEMBERS.into(), + proposal.get_dispatch_info().weight, + *length_bound as Weight, + ), + DispatchClass::Operational + )] + fn execute(origin, + proposal: Box<>::Proposal>, + #[compact] length_bound: u32, + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), Error::::NotMember); + let members = Self::members(); + ensure!(members.contains(&who), Error::::NotMember); + let proposal_len = proposal.using_encoded(|x| x.len()); + ensure!(proposal_len <= length_bound as usize, Error::::WrongProposalLength); let proposal_hash = T::Hashing::hash_of(&proposal); - let ok = proposal.dispatch(RawOrigin::Member(who).into()).is_ok(); - Self::deposit_event(RawEvent::MemberExecuted(proposal_hash, ok)); + let result = proposal.dispatch(RawOrigin::Member(who).into()); + Self::deposit_event( + RawEvent::MemberExecuted(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + ); + + Ok(get_result_weight(result).map(|w| weight_for::execute::( + members.len() as Weight, + w, + proposal_len as Weight + )).into()) } + /// Add a new proposal to either be voted on or executed directly. + /// + /// Requires the sender to be member. + /// + /// `threshold` determines whether `proposal` is executed directly (`threshold < 2`) + /// or put up for voting. + /// /// # - /// - Bounded storage reads and writes. - /// - Argument `threshold` has bearing on weight. + /// ## Weight + /// - `O(B + M + P1)` or `O(B + M + P2)` where: + /// - `B` is `proposal` size in bytes (length-fee-bounded) + /// - `M` is members-count (code- and governance-bounded) + /// - branching is influenced by `threshold` where: + /// - `P1` is proposal execution complexity (`threshold < 2`) + /// - `P2` is proposals-count (code-bounded) (`threshold >= 2`) + /// - DB: + /// - 1 storage read `is_member` (codec `O(M)`) + /// - 1 storage read `ProposalOf::contains_key` (codec `O(1)`) + /// - DB accesses influenced by `threshold`: + /// - EITHER storage accesses done by `proposal` (`threshold < 2`) + /// - OR proposal insertion (`threshold <= 2`) + /// - 1 storage mutation `Proposals` (codec `O(P2)`) + /// - 1 storage mutation `ProposalCount` (codec `O(1)`) + /// - 1 storage write `ProposalOf` (codec `O(B)`) + /// - 1 storage write `Voting` (codec `O(M)`) + /// - 1 event /// # - #[weight = (5_000_000_000, DispatchClass::Operational)] - fn propose(origin, #[compact] threshold: MemberCount, proposal: Box<>::Proposal>) { + #[weight = ( + if *threshold < 2 { + weight_for::propose_execute::( + MAX_MEMBERS.into(), // M + proposal.get_dispatch_info().weight, // P1 + *length_bound as Weight, // B + ) + } else { + weight_for::propose_proposed::( + MAX_MEMBERS.into(), // M + T::MaxProposals::get().into(), // P2 + *length_bound as Weight, // B + ) + }, + DispatchClass::Operational + )] + fn propose(origin, + #[compact] threshold: MemberCount, + proposal: Box<>::Proposal>, + #[compact] length_bound: u32 + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), Error::::NotMember); + let members = Self::members(); + ensure!(members.contains(&who), Error::::NotMember); + let proposal_len = proposal.using_encoded(|x| x.len()); + ensure!(proposal_len <= length_bound as usize, Error::::WrongProposalLength); let proposal_hash = T::Hashing::hash_of(&proposal); - ensure!(!>::contains_key(proposal_hash), Error::::DuplicateProposal); if threshold < 2 { let seats = Self::members().len() as MemberCount; - let ok = proposal.dispatch(RawOrigin::Members(1, seats).into()).is_ok(); - Self::deposit_event(RawEvent::Executed(proposal_hash, ok)); + let result = proposal.dispatch(RawOrigin::Members(1, seats).into()); + Self::deposit_event( + RawEvent::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + ); + + Ok(get_result_weight(result).map(|w| weight_for::propose_execute::( + members.len() as Weight, // M + w, // P1 + proposal_len as Weight, // B + )).into()) } else { + let active_proposals = + >::try_mutate(|proposals| -> Result { + proposals.push(proposal_hash); + ensure!( + proposals.len() <= T::MaxProposals::get() as usize, + Error::::TooManyProposals + ); + Ok(proposals.len()) + })?; let index = Self::proposal_count(); >::mutate(|i| *i += 1); - >::mutate(|proposals| proposals.push(proposal_hash)); >::insert(proposal_hash, *proposal); let end = system::Module::::block_number() + T::MotionDuration::get(); let votes = Votes { index, threshold, ayes: vec![who.clone()], nays: vec![], end }; >::insert(proposal_hash, votes); Self::deposit_event(RawEvent::Proposed(who, index, proposal_hash, threshold)); + + Ok(Some(weight_for::propose_proposed::( + members.len() as Weight, // M + active_proposals as Weight, // P2 + proposal_len as Weight, // B + )).into()) } } + /// Add an aye or nay vote for the sender to the given proposal. + /// + /// Requires the sender to be a member. + /// /// # - /// - Bounded storage read and writes. - /// - Will be slightly heavier if the proposal is approved / disapproved after the vote. + /// ## Weight + /// - `O(M)` where `M` is members-count (code- and governance-bounded) + /// - DB: + /// - 1 storage read `Members` (codec `O(M)`) + /// - 1 storage mutation `Voting` (codec `O(M)`) + /// - 1 event /// # - #[weight = (200_000_000, DispatchClass::Operational)] - fn vote(origin, proposal: T::Hash, #[compact] index: ProposalIndex, approve: bool) { + #[weight = ( + weight_for::vote::(MAX_MEMBERS.into()), + DispatchClass::Operational + )] + fn vote(origin, + proposal: T::Hash, + #[compact] index: ProposalIndex, + approve: bool, + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - ensure!(Self::is_member(&who), Error::::NotMember); + let members = Self::members(); + ensure!(members.contains(&who), Error::::NotMember); let mut voting = Self::voting(&proposal).ok_or(Error::::ProposalMissing)?; ensure!(voting.index == index, Error::::WrongIndex); @@ -279,64 +599,134 @@ decl_module! { let no_votes = voting.nays.len() as MemberCount; Self::deposit_event(RawEvent::Voted(who, proposal, approve, yes_votes, no_votes)); - let seats = Self::members().len() as MemberCount; + Voting::::insert(&proposal, voting); - let approved = yes_votes >= voting.threshold; - let disapproved = seats.saturating_sub(no_votes) < voting.threshold; - if approved || disapproved { - Self::finalize_proposal(approved, seats, voting, proposal); - } else { - Voting::::insert(&proposal, voting); - } + Ok(Some(weight_for::vote::(members.len() as Weight)).into()) } - /// May be called by any signed account after the voting duration has ended in order to - /// finish voting and close the proposal. + /// Close a vote that is either approved, disapproved or whose voting period has ended. /// - /// Abstentions are counted as rejections unless there is a prime member set and the prime - /// member cast an approval. + /// May be called by any signed account in order to finish voting and close the proposal. /// - /// - the weight of `proposal` preimage. - /// - up to three events deposited. - /// - one read, two removals, one mutation. (plus three static reads.) - /// - computation and i/o `O(P + L + M)` where: - /// - `M` is number of members, - /// - `P` is number of active proposals, - /// - `L` is the encoded length of `proposal` preimage. - #[weight = (200_000_000, DispatchClass::Operational)] - fn close(origin, proposal: T::Hash, #[compact] index: ProposalIndex) { + /// If called before the end of the voting period it will only close the vote if it is + /// has enough votes to be approved or disapproved. + /// + /// If called after the end of the voting period abstentions are counted as rejections + /// unless there is a prime member set and the prime member cast an approval. + /// + /// + `proposal_weight_bound`: The maximum amount of weight consumed by executing the closed proposal. + /// + `length_bound`: The upper bound for the length of the proposal in storage. Checked via + /// `storage::read` so it is `size_of::() == 4` larger than the pure length. + /// + /// # + /// ## Weight + /// - `O(B + M + P1 + P2)` where: + /// - `B` is `proposal` size in bytes (length-fee-bounded) + /// - `M` is members-count (code- and governance-bounded) + /// - `P1` is the complexity of `proposal` preimage. + /// - `P2` is proposal-count (code-bounded) + /// - DB: + /// - 2 storage reads (`Members`: codec `O(M)`, `Prime`: codec `O(1)`) + /// - 3 mutations (`Voting`: codec `O(M)`, `ProposalOf`: codec `O(B)`, `Proposals`: codec `O(P2)`) + /// - any mutations done while executing `proposal` (`P1`) + /// - up to 3 events + /// # + #[weight = ( + weight_for::close::( + MAX_MEMBERS.into(), // `M` + *proposal_weight_bound, // `P1` + T::MaxProposals::get().into(), // `P2` + *length_bound as Weight, // B + ), + DispatchClass::Operational + )] + fn close(origin, + proposal: T::Hash, + #[compact] index: ProposalIndex, + #[compact] proposal_weight_bound: Weight, + #[compact] length_bound: u32 + ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; let voting = Self::voting(&proposal).ok_or(Error::::ProposalMissing)?; ensure!(voting.index == index, Error::::WrongIndex); - ensure!(system::Module::::block_number() >= voting.end, Error::::TooEarly); - - // default to true only if there's a prime and they voted in favour. - let default = Self::prime().map_or( - false, - |who| voting.ayes.iter().any(|a| a == &who), - ); let mut no_votes = voting.nays.len() as MemberCount; let mut yes_votes = voting.ayes.len() as MemberCount; let seats = Self::members().len() as MemberCount; + let approved = yes_votes >= voting.threshold; + let disapproved = seats.saturating_sub(no_votes) < voting.threshold; + // Allow (dis-)approving the proposal as soon as there are enough votes. + if approved || disapproved { + let (p, len) = Self::validate_and_get_proposal( + &proposal, + length_bound, + proposal_weight_bound + )?; + let finalize_weight = Self::finalize_proposal(approved, seats, voting, proposal, p); + return Ok(Some( + weight_for::close_without_finalize::(seats.into(), len as Weight) + .saturating_add(finalize_weight) + ).into()); + } + + // Only allow actual closing of the proposal after the voting period has ended. + ensure!(system::Module::::block_number() >= voting.end, Error::::TooEarly); + + // default to true only if there's a prime and they voted in favour. + let default = Self::prime().map_or(false, |who| voting.ayes.iter().any(|a| a == &who)); + let abstentions = seats - (yes_votes + no_votes); match default { true => yes_votes += abstentions, false => no_votes += abstentions, } + let approved = yes_votes >= voting.threshold; + let (p, len) = Self::validate_and_get_proposal( + &proposal, + length_bound, + proposal_weight_bound + )?; Self::deposit_event(RawEvent::Closed(proposal, yes_votes, no_votes)); - Self::finalize_proposal(yes_votes >= voting.threshold, seats, voting, proposal); + let finalize_weight = Self::finalize_proposal(approved, seats, voting, proposal, p); + Ok(Some( + weight_for::close_without_finalize::(seats.into(), len as Weight) + .saturating_add(T::DbWeight::get().reads(1)) // read `Prime` + .saturating_add(finalize_weight) + ).into()) } } } impl, I: Instance> Module { + /// Check whether `who` is a member of the collective. pub fn is_member(who: &T::AccountId) -> bool { + // Note: The dispatchables *do not* use this to check membership so make sure + // to update those if this is changed. Self::members().contains(who) } + /// Ensure that the right proposal bounds were passed and get the proposal from storage. + /// + /// Checks the length in storage via `storage::read` which adds an extra `size_of::() == 4` + /// to the length. + fn validate_and_get_proposal( + hash: &T::Hash, + length_bound: u32, + weight_bound: Weight + ) -> Result<(>::Proposal, usize), DispatchError> { + let key = ProposalOf::::hashed_key_for(hash); + // read the length of the proposal storage entry directly + let proposal_len = storage::read(&key, &mut [0; 0], 0) + .ok_or(Error::::ProposalMissing)?; + ensure!(proposal_len <= length_bound, Error::::WrongProposalLength); + let proposal = ProposalOf::::get(hash).ok_or(Error::::ProposalMissing)?; + let proposal_weight = proposal.get_dispatch_info().weight; + ensure!(proposal_weight <= weight_bound, Error::::WrongProposalWeight); + Ok((proposal, proposal_len as usize)) + } + /// Weight: /// If `approved`: /// - the weight of `proposal` preimage. @@ -355,35 +745,71 @@ impl, I: Instance> Module { approved: bool, seats: MemberCount, voting: Votes, - proposal: T::Hash, - ) { + proposal_hash: T::Hash, + proposal: >::Proposal, + ) -> Weight { + let db = T::DbWeight::get(); + let mut weight: Weight = 0; if approved { - Self::deposit_event(RawEvent::Approved(proposal)); + Self::deposit_event(RawEvent::Approved(proposal_hash)); - // execute motion, assuming it exists. - if let Some(p) = ProposalOf::::take(&proposal) { - let origin = RawOrigin::Members(voting.threshold, seats).into(); - let ok = p.dispatch(origin).is_ok(); - Self::deposit_event(RawEvent::Executed(proposal, ok)); - } + let dispatch_weight = proposal.get_dispatch_info().weight; + let origin = RawOrigin::Members(voting.threshold, seats).into(); + let result = proposal.dispatch(origin); + Self::deposit_event( + RawEvent::Executed(proposal_hash, result.map(|_| ()).map_err(|e| e.error)) + ); + weight = weight.saturating_add( + // default to the dispatch info weight for safety + get_result_weight(result).unwrap_or(dispatch_weight) // P1 + ); } else { // disapproved - ProposalOf::::remove(&proposal); - Self::deposit_event(RawEvent::Disapproved(proposal)); + Self::deposit_event(RawEvent::Disapproved(proposal_hash)); } - // remove vote - Voting::::remove(&proposal); - Proposals::::mutate(|proposals| proposals.retain(|h| h != &proposal)); + // remove proposal and vote + ProposalOf::::remove(&proposal_hash); + Voting::::remove(&proposal_hash); + let num_proposals = Proposals::::mutate(|proposals| { + proposals.retain(|h| h != &proposal_hash); + proposals.len() + 1 // calculate weight based on original length + }); + weight.saturating_add(db.reads_writes(1, 3)) // `Voting`, `Proposals`, `ProposalOf` + .saturating_add(490_000 * num_proposals as Weight) // P2 } } impl, I: Instance> ChangeMembers for Module { + /// Update the members of the collective. Votes are updated and the prime is reset. + /// + /// NOTE: Does not enforce the expected `MAX_MEMBERS` limit on the amount of members, but + /// the weight estimations rely on it to estimate dispatchable weight. + /// + /// # + /// ## Weight + /// - `O(MP + N)` + /// - where `M` old-members-count (governance-bounded) + /// - where `N` new-members-count (governance-bounded) + /// - where `P` proposals-count + /// - DB: + /// - 1 storage read (codec `O(P)`) for reading the proposals + /// - `P` storage mutations for updating the votes (codec `O(M)`) + /// - 1 storage write (codec `O(N)`) for storing the new members + /// - 1 storage write (codec `O(1)`) for deleting the old prime + /// # fn change_members_sorted( _incoming: &[T::AccountId], outgoing: &[T::AccountId], new: &[T::AccountId], ) { + if new.len() > MAX_MEMBERS as usize { + debug::error!( + "New members count exceeds maximum amount of members expected. (expected: {}, actual: {})", + MAX_MEMBERS, + new.len() + ); + } // remove accounts from all current voting in motions. let mut outgoing = outgoing.to_vec(); outgoing.sort_unstable(); @@ -539,6 +965,7 @@ mod tests { pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); pub const MotionDuration: u64 = 3; + pub const MaxProposals: u32 = 100; } impl frame_system::Trait for Test { type Origin = Origin; @@ -569,12 +996,14 @@ mod tests { type Proposal = Call; type Event = Event; type MotionDuration = MotionDuration; + type MaxProposals = MaxProposals; } impl Trait for Test { type Origin = Origin; type Proposal = Call; type Event = Event; type MotionDuration = MotionDuration; + type MaxProposals = MaxProposals; } pub type Block = sp_runtime::generic::Block; @@ -622,17 +1051,17 @@ mod tests { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); System::set_block_number(3); assert_noop!( - Collective::close(Origin::signed(4), hash.clone(), 0), + Collective::close(Origin::signed(4), hash.clone(), 0, Weight::max_value(), u32::max_value()), Error::::TooEarly ); System::set_block_number(4); - assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0)); + assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0, Weight::max_value(), u32::max_value())); let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] }; assert_eq!(System::events(), vec![ @@ -644,18 +1073,37 @@ mod tests { }); } + #[test] + fn proposal_weight_limit_works() { + new_test_ext().execute_with(|| { + let proposal = Call::Collective(crate::Call::set_members(vec![1, 2, 3], None, MAX_MEMBERS)); + let hash = BlakeTwo256::hash_of(&proposal); + + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); + assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); + + System::set_block_number(4); + let proposal_weight = proposal.get_dispatch_info().weight; + assert_noop!( + Collective::close(Origin::signed(4), hash.clone(), 0, proposal_weight - 100, u32::max_value()), + Error::::WrongProposalWeight + ); + assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0, Weight::max_value(), u32::max_value())); + }) + } + #[test] fn close_with_prime_works() { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); - assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(3))); + assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(3), MAX_MEMBERS)); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); System::set_block_number(4); - assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0)); + assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0, Weight::max_value(), u32::max_value())); let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] }; assert_eq!(System::events(), vec![ @@ -672,13 +1120,13 @@ mod tests { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); - assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(1))); + assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(1), MAX_MEMBERS)); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); System::set_block_number(4); - assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0)); + assert_ok!(Collective::close(Origin::signed(4), hash.clone(), 0, Weight::max_value(), u32::max_value())); let record = |event| EventRecord { phase: Phase::Initialization, event, topics: vec![] }; assert_eq!(System::events(), vec![ @@ -686,7 +1134,7 @@ mod tests { record(Event::collective_Instance1(RawEvent::Voted(2, hash.clone(), true, 2, 0))), record(Event::collective_Instance1(RawEvent::Closed(hash.clone(), 3, 0))), record(Event::collective_Instance1(RawEvent::Approved(hash.clone()))), - record(Event::collective_Instance1(RawEvent::Executed(hash.clone(), false))) + record(Event::collective_Instance1(RawEvent::Executed(hash.clone(), Err(DispatchError::BadOrigin)))) ]); }); } @@ -697,7 +1145,7 @@ mod tests { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); let end = 4; - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); assert_eq!( Collective::voting(&hash), @@ -711,7 +1159,7 @@ mod tests { let proposal = make_proposal(69); let hash = BlakeTwo256::hash_of(&proposal); - assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false)); assert_eq!( Collective::voting(&hash), @@ -731,13 +1179,13 @@ mod tests { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); let end = 4; - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); assert_eq!( Collective::voting(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![1, 2], nays: vec![], end }) ); - assert_ok!(Collective::set_members(Origin::ROOT, vec![2, 3, 4], None)); + assert_ok!(Collective::set_members(Origin::ROOT, vec![2, 3, 4], None, MAX_MEMBERS)); assert_eq!( Collective::voting(&hash), Some(Votes { index: 0, threshold: 3, ayes: vec![2], nays: vec![], end }) @@ -745,13 +1193,13 @@ mod tests { let proposal = make_proposal(69); let hash = BlakeTwo256::hash_of(&proposal); - assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(2), 2, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(3), hash.clone(), 1, false)); assert_eq!( Collective::voting(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![3], end }) ); - assert_ok!(Collective::set_members(Origin::ROOT, vec![2, 4], None)); + assert_ok!(Collective::set_members(Origin::ROOT, vec![2, 4], None, MAX_MEMBERS)); assert_eq!( Collective::voting(&hash), Some(Votes { index: 1, threshold: 2, ayes: vec![2], nays: vec![], end }) @@ -765,7 +1213,7 @@ mod tests { let proposal = make_proposal(42); let hash = proposal.blake2_256().into(); let end = 4; - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_eq!(Collective::proposals(), vec![hash]); assert_eq!(Collective::proposal_of(&hash), Some(proposal)); assert_eq!( @@ -788,12 +1236,56 @@ mod tests { }); } + #[test] + fn limit_active_proposals() { + new_test_ext().execute_with(|| { + for i in 0..MaxProposals::get() { + let proposal = make_proposal(i as u64); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); + } + let proposal = make_proposal(MaxProposals::get() as u64 + 1); + assert_noop!( + Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value()), + Error::::TooManyProposals + ); + }) + } + + #[test] + fn correct_validate_and_get_proposal() { + new_test_ext().execute_with(|| { + let proposal = Call::Collective(crate::Call::set_members(vec![1, 2, 3], None, MAX_MEMBERS)); + let length = proposal.encode().len() as u32; + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), length)); + + let hash = BlakeTwo256::hash_of(&proposal); + let weight = proposal.get_dispatch_info().weight; + assert_noop!( + Collective::validate_and_get_proposal(&BlakeTwo256::hash_of(&vec![3; 4]), length, weight), + Error::::ProposalMissing + ); + assert_noop!( + Collective::validate_and_get_proposal(&hash, length - 2, weight), + Error::::WrongProposalLength + ); + assert_noop!( + Collective::validate_and_get_proposal(&hash, length, weight - 10), + Error::::WrongProposalWeight + ); + let res = Collective::validate_and_get_proposal(&hash, length, weight); + assert_ok!(res.clone()); + let (retrieved_proposal, len) = res.unwrap(); + assert_eq!(length as usize, len); + assert_eq!(proposal, retrieved_proposal); + }) + } + #[test] fn motions_ignoring_non_collective_proposals_works() { new_test_ext().execute_with(|| { let proposal = make_proposal(42); assert_noop!( - Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone())), + Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone()), u32::max_value()), Error::::NotMember ); }); @@ -804,7 +1296,7 @@ mod tests { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_noop!( Collective::vote(Origin::signed(42), hash.clone(), 0, true), Error::::NotMember, @@ -818,7 +1310,7 @@ mod tests { System::set_block_number(3); let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_noop!( Collective::vote(Origin::signed(2), hash.clone(), 1, true), Error::::WrongIndex, @@ -832,7 +1324,7 @@ mod tests { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); let end = 4; - assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), u32::max_value())); assert_eq!( Collective::voting(&hash), Some(Votes { index: 0, threshold: 2, ayes: vec![1], nays: vec![], end }) @@ -882,10 +1374,11 @@ mod tests { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, false)); + assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, Weight::max_value(), u32::max_value())); assert_eq!(Collective::proposals(), vec![]); - assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), u32::max_value())); assert_eq!(Collective::proposals(), vec![hash]); }); } @@ -895,8 +1388,9 @@ mod tests { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); - assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, false)); + assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, Weight::max_value(), u32::max_value())); assert_eq!(System::events(), vec![ EventRecord { @@ -937,8 +1431,9 @@ mod tests { new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); - assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()))); + assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone()), u32::max_value())); assert_ok!(Collective::vote(Origin::signed(2), hash.clone(), 0, true)); + assert_ok!(Collective::close(Origin::signed(2), hash.clone(), 0, Weight::max_value(), u32::max_value())); assert_eq!(System::events(), vec![ EventRecord { @@ -973,7 +1468,7 @@ mod tests { phase: Phase::Initialization, event: Event::collective_Instance1(RawEvent::Executed( hex!["68eea8f20b542ec656c6ac2d10435ae3bd1729efc34d1354ab85af840aad2d35"].into(), - false, + Err(DispatchError::BadOrigin), )), topics: vec![], }