diff --git a/Cargo.lock b/Cargo.lock index f2296d442ed6e..2db678150242b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4163,6 +4163,7 @@ dependencies = [ "sp-runtime", "sp-std", "sp-storage", + "substrate-test-utils", ] [[package]] diff --git a/frame/democracy/Cargo.toml b/frame/democracy/Cargo.toml index 83caa671cee95..f0d2867f22492 100644 --- a/frame/democracy/Cargo.toml +++ b/frame/democracy/Cargo.toml @@ -26,6 +26,7 @@ sp-core = { version = "2.0.0-dev", path = "../../primitives/core" } pallet-balances = { version = "2.0.0-dev", path = "../balances" } pallet-scheduler = { version = "2.0.0-dev", path = "../scheduler" } sp-storage = { version = "2.0.0-dev", path = "../../primitives/storage" } +substrate-test-utils = { version = "2.0.0-dev", path = "../../test-utils" } hex-literal = "0.2.1" [features] @@ -43,5 +44,6 @@ std = [ runtime-benchmarks = [ "frame-benchmarking", "frame-system/runtime-benchmarks", + "frame-support/runtime-benchmarks", "sp-runtime/runtime-benchmarks", ] diff --git a/frame/democracy/src/benchmarking.rs b/frame/democracy/src/benchmarking.rs index 4459a1aaa5410..f318e0b8eac60 100644 --- a/frame/democracy/src/benchmarking.rs +++ b/frame/democracy/src/benchmarking.rs @@ -52,7 +52,12 @@ fn add_proposal(n: u32) -> Result { let value = T::MinimumDeposit::get(); let proposal_hash: T::Hash = T::Hashing::hash_of(&n); - Democracy::::propose(RawOrigin::Signed(other).into(), proposal_hash, value.into())?; + Democracy::::propose( + RawOrigin::Signed(other).into(), + proposal_hash, + value.into(), + u32::max_value(), + )?; Ok(proposal_hash) } @@ -116,7 +121,7 @@ benchmarks! { let caller = funded_account::("caller", 0); let proposal_hash: T::Hash = T::Hashing::hash_of(&p); let value = T::MinimumDeposit::get(); - }: _(RawOrigin::Signed(caller), proposal_hash, value.into()) + }: _(RawOrigin::Signed(caller), proposal_hash, value.into(), u32::max_value()) verify { assert_eq!(Democracy::::public_props().len(), (p + 1) as usize, "Proposals not created."); } @@ -130,15 +135,15 @@ benchmarks! { // Create s existing "seconds" for i in 0 .. s { let seconder = funded_account::("seconder", i); - Democracy::::second(RawOrigin::Signed(seconder).into(), 0)?; + Democracy::::second(RawOrigin::Signed(seconder).into(), 0, u32::max_value())?; } let deposits = Democracy::::deposit_of(0).ok_or("Proposal not created")?; - assert_eq!(deposits.1.len(), (s + 1) as usize, "Seconds not recorded"); - }: _(RawOrigin::Signed(caller), 0) + assert_eq!(deposits.0.len(), (s + 1) as usize, "Seconds not recorded"); + }: _(RawOrigin::Signed(caller), 0, u32::max_value()) verify { let deposits = Democracy::::deposit_of(0).ok_or("Proposal not created")?; - assert_eq!(deposits.1.len(), (s + 2) as usize, "`second` benchmark did not work"); + assert_eq!(deposits.0.len(), (s + 2) as usize, "`second` benchmark did not work"); } vote_new { @@ -296,13 +301,14 @@ benchmarks! { // Worst case scenario, we external propose a previously blacklisted proposal external_propose { let p in 1 .. MAX_PROPOSALS; + let v in 1 .. MAX_VETOERS; let origin = T::ExternalOrigin::successful_origin(); let proposal_hash = T::Hashing::hash_of(&p); // Add proposal to blacklist with block number 0 Blacklist::::insert( proposal_hash, - (T::BlockNumber::zero(), vec![T::AccountId::default()]) + (T::BlockNumber::zero(), (0..v).map(|_| T::AccountId::default()).collect::>()) ); let call = Call::::external_propose(proposal_hash); @@ -650,7 +656,7 @@ benchmarks! { assert!(Preimages::::contains_key(proposal_hash)); let caller = funded_account::("caller", 0); - }: _(RawOrigin::Signed(caller), proposal_hash.clone()) + }: _(RawOrigin::Signed(caller), proposal_hash.clone(), u32::max_value()) verify { let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); assert!(!Preimages::::contains_key(proposal_hash)); diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index ddc3b36f4c877..c7c0774c9ee63 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -168,14 +168,15 @@ use sp_runtime::{ DispatchResult, DispatchError, RuntimeDebug, traits::{Zero, Hash, Dispatchable, Saturating}, }; -use codec::{Ref, Encode, Decode}; +use codec::{Ref, Encode, Decode, Input}; use frame_support::{ decl_module, decl_storage, decl_event, decl_error, ensure, Parameter, + storage::IterableStorageMap, weights::{Weight, DispatchClass}, traits::{ Currency, ReservableCurrency, LockableCurrency, WithdrawReason, LockIdentifier, Get, OnUnbalanced, BalanceStatus, schedule::Named as ScheduleNamed, EnsureOrigin - } + }, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -277,6 +278,15 @@ pub trait Trait: frame_system::Trait + Sized { /// The Scheduler. type Scheduler: ScheduleNamed; + + /// The maximum number of possiblevetoers origin, not a hard limit, only used to compute weight. + type MaxVetoers: Get; + + /// The maximum number of vote for an account. + /// + /// Also used to compute weight, a too big value can + /// lead to extrinsic with very big weight: see `delegate` for instance. + type MaxVotes: Get; } #[derive(Clone, Encode, Decode, RuntimeDebug)] @@ -303,6 +313,21 @@ impl PreimageStatus Self { + Releases::V1 + } +} + decl_storage! { trait Store for Module as Democracy { // TODO: Refactor public proposal queue into its own pallet. @@ -313,7 +338,7 @@ decl_storage! { pub PublicProps get(fn public_props): Vec<(PropIndex, T::Hash, T::AccountId)>; /// Those who have locked a deposit. pub DepositOf get(fn deposit_of): - map hasher(twox_64_concat) PropIndex => Option<(BalanceOf, Vec)>; + map hasher(twox_64_concat) PropIndex => Option<(Vec, BalanceOf)>; /// Map of hashes to the proposal preimage, along with who registered it and their deposit. /// The block number is the block at which it was deposited. @@ -367,6 +392,11 @@ decl_storage! { /// Record of all proposals that have been subject to emergency cancellation. pub Cancellations: map hasher(identity) T::Hash => bool; + + /// Storage version of the pallet. + /// + /// New networks start with last version. + StorageVersion build(|_| Releases::V2): Releases; } } @@ -491,6 +521,10 @@ decl_error! { InstantNotAllowed, /// Delegation to oneself makes no sense. Nonsense, + /// Invalid upper bound. + WrongUpperBound, + /// Maximum number of votes reached. + MaxVotesReached, } } @@ -525,6 +559,22 @@ decl_module! { fn deposit_event() = default; + fn on_runtime_upgrade() -> Weight { + if StorageVersion::get() == Releases::V1 { + DepositOf::::translate::< + (BalanceOf, Vec), _ + >(|_, (balance, accounts)| { + Some((accounts, balance)) + }); + + StorageVersion::put(Releases::V2); + + T::MaximumBlockWeight::get() + } else { + T::DbWeight::get().reads(1) + } + } + /// Propose a sensitive action to be taken. /// /// The dispatch origin of this call must be _Signed_ and the sender must @@ -532,18 +582,24 @@ decl_module! { /// /// - `proposal_hash`: The hash of the proposal preimage. /// - `value`: The amount of deposit (must be at least `MinimumDeposit`). + /// - `proposals_upper_bound`: an upper bound on the current number of public proposals. + /// Extrinsic is weighted according to this value with no refund. /// /// Emits `Proposed`. /// /// # - /// - `O(P)` - /// - P is the number proposals in the `PublicProps` vec. - /// - Two DB changes, one DB entry. + /// - Complexity: `O(P)` where `P` is the number of proposals in the `PublicProps` vec. + /// - Db reads: `PublicPropCount`, `PublicProps` + /// - Db writes: `PublicPropCount`, `PublicProps`, `DepositOf` /// # - #[weight = 5_000_000_000] + #[weight = 51_000_000 + .saturating_add(220_000.saturating_mul(Weight::from(*proposals_upper_bound))) + .saturating_add(T::DbWeight::get().reads_writes(2, 3)) + ] fn propose(origin, proposal_hash: T::Hash, - #[compact] value: BalanceOf + #[compact] value: BalanceOf, + proposals_upper_bound: u32, ) { let who = ensure_signed(origin)?; ensure!(value >= T::MinimumDeposit::get(), Error::::ValueLow); @@ -551,9 +607,10 @@ decl_module! { let index = Self::public_prop_count(); PublicPropCount::put(index + 1); - >::insert(index, (value, &[&who][..])); + >::insert(index, (&[&who][..], value)); let new_prop = (index, proposal_hash, who); + ensure!(Self::len_of_public_props() <= proposals_upper_bound, Error::::WrongUpperBound); >::append_or_put(&[Ref::from(&new_prop)][..]); Self::deposit_event(RawEvent::Proposed(index, value)); @@ -565,19 +622,28 @@ decl_module! { /// must have funds to cover the deposit, equal to the original deposit. /// /// - `proposal`: The index of the proposal to second. + /// - `seconds_upper_bound`: an upper bound on the current number of seconds on this + /// proposal. Extrinsic is weighted according to this value with no refund. /// /// # - /// - `O(S)`. - /// - S is the number of seconds a proposal already has. - /// - One DB entry. + /// - Complexity: `O(S)` where S is the number of seconds a proposal already has. + /// - Db reads: `DepositOf` + /// - Db writes: `DepositOf` /// # - #[weight = 5_000_000_000] - fn second(origin, #[compact] proposal: PropIndex) { + #[weight = 31_000_000 + .saturating_add(190_000.saturating_mul(Weight::from(*seconds_upper_bound))) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + ] + fn second(origin, #[compact] proposal: PropIndex, seconds_upper_bound: u32) { let who = ensure_signed(origin)?; + + let seconds = Self::len_of_deposit_of(proposal) + .ok_or_else(|| Error::::ProposalMissing)?; + ensure!(seconds <= seconds_upper_bound, Error::::WrongUpperBound); let mut deposit = Self::deposit_of(proposal) .ok_or(Error::::ProposalMissing)?; - T::Currency::reserve(&who, deposit.0)?; - deposit.1.push(who); + T::Currency::reserve(&who, deposit.1)?; + deposit.0.push(who); >::insert(proposal, deposit); } @@ -590,11 +656,12 @@ decl_module! { /// - `vote`: The vote configuration. /// /// # - /// - `O(R)`. - /// - R is the number of referendums the voter has voted on. - /// - One DB change, one DB entry. + /// - Complexity: `O(R)` where R is the number of referendums the voter has voted on. + /// weight is charged as if maximum votes. + /// - Db reads: `ReferendumInfoOf`, `VotingOf`, `balances locks` + /// - Db writes: `ReferendumInfoOf`, `VotingOf`, `balances locks` /// # - #[weight = 200_000_000] + #[weight = 64_000_000 + 220_000 * Weight::from(T::MaxVotes::get()) + T::DbWeight::get().reads_writes(3, 3)] fn vote(origin, #[compact] ref_index: ReferendumIndex, vote: AccountVote>, @@ -612,10 +679,12 @@ decl_module! { /// - `vote`: The vote configuration. /// /// # - /// - `O(1)`. - /// - One DB change, one DB entry. + /// - Complexity: `O(R)` where R is the number of referendums the proxy has voted on. + /// weight is charged as if maximum votes. + /// - Db reads: `ReferendumInfoOf`, `VotingOf`, `balances locks`, `Proxy`, `proxy account` + /// - Db writes: `ReferendumInfoOf`, `VotingOf`, `balances locks` /// # - #[weight = 200_000_000] + #[weight = 73_000_000 + 220_000 * Weight::from(T::MaxVotes::get()) + T::DbWeight::get().reads_writes(5, 3)] fn proxy_vote(origin, #[compact] ref_index: ReferendumIndex, vote: AccountVote>, @@ -633,9 +702,11 @@ decl_module! { /// -`ref_index`: The index of the referendum to cancel. /// /// # - /// - `O(1)`. + /// - Complexity: `O(1)`. + /// - Db reads: `ReferendumInfoOf`, `Cancellations` + /// - Db writes: `ReferendumInfoOf`, `Cancellations` /// # - #[weight = (500_000_000, DispatchClass::Operational)] + #[weight = (45_000_000 + T::DbWeight::get().reads_writes(2, 2), DispatchClass::Operational)] fn emergency_cancel(origin, ref_index: ReferendumIndex) { T::CancellationOrigin::ensure_origin(origin)?; @@ -655,9 +726,12 @@ decl_module! { /// - `proposal_hash`: The preimage hash of the proposal. /// /// # - /// - `O(1)`. - /// - One DB change. + /// - Complexity `O(V)` with V number of vetoers in the blacklist of proposal. + /// Decoding vec of length V. Charged as maximum + /// - Db reads: `NextExternal`, `Blacklist` + /// - Db writes: `NextExternal` /// # + // TODO TODO: weight according to result of new benchmark #[weight = 5_000_000_000] fn external_propose(origin, proposal_hash: T::Hash) { T::ExternalOrigin::ensure_origin(origin)?; @@ -682,10 +756,10 @@ decl_module! { /// pre-scheduled `external_propose` call. /// /// # - /// - `O(1)`. - /// - One DB change. + /// - Complexity: `O(1)` + /// - Db write: `NextExternal` /// # - #[weight = 5_000_000_000] + #[weight = 4_000_000 + T::DbWeight::get().writes(1)] fn external_propose_majority(origin, proposal_hash: T::Hash) { T::ExternalMajorityOrigin::ensure_origin(origin)?; >::put((proposal_hash, VoteThreshold::SimpleMajority)); @@ -702,10 +776,10 @@ decl_module! { /// pre-scheduled `external_propose` call. /// /// # - /// - `O(1)`. - /// - One DB change. + /// - Complexity: `O(1)` + /// - Db write: `NextExternal` /// # - #[weight = 5_000_000_000] + #[weight = 4_000_000 + T::DbWeight::get().writes(1)] fn external_propose_default(origin, proposal_hash: T::Hash) { T::ExternalDefaultOrigin::ensure_origin(origin)?; >::put((proposal_hash, VoteThreshold::SuperMajorityAgainst)); @@ -726,11 +800,11 @@ decl_module! { /// Emits `Started`. /// /// # - /// - One DB clear. - /// - One DB change. - /// - One extra DB entry. + /// - Complexity: `O(1)` + /// - Db reads: `NextExternal`, `ReferendumCount` + /// - Db writes: `NextExternal`, `ReferendumCount`, `ReferendumInfoOf` /// # - #[weight = 200_000_000] + #[weight = 32_000_000 + T::DbWeight::get().reads_writes(2, 3)] fn fast_track(origin, proposal_hash: T::Hash, voting_period: T::BlockNumber, @@ -775,13 +849,13 @@ decl_module! { /// Emits `Vetoed`. /// /// # - /// - Two DB entries. - /// - One DB clear. - /// - Performs a binary search on `existing_vetoers` which should not - /// be very large. - /// - O(log v), v is number of `existing_vetoers` + /// - Complexity: `O(V + log(V))` where V is number of `existing vetoers` + /// Performs a binary search on `existing_vetoers` which should not be very large. + /// - Db reads: `NextExternal`, `Blacklist` + /// - Db writes: `NextExternal`, `Blacklist` /// # - #[weight = 200_000_000] + #[weight = 34_000_000 + 180_000 * Weight::from(T::MaxVetoers::get()) + + T::DbWeight::get().reads_writes(2, 2)] fn veto_external(origin, proposal_hash: T::Hash) { let who = T::VetoOrigin::ensure_origin(origin)?; @@ -812,9 +886,10 @@ decl_module! { /// - `ref_index`: The index of the referendum to cancel. /// /// # - /// - `O(1)`. + /// - Complexity: `O(1)`. + /// - Db writes: `ReferendumInfoOf` /// # - #[weight = (0, DispatchClass::Operational)] + #[weight = (50_000_000 + T::DbWeight::get().writes(1), DispatchClass::Operational)] fn cancel_referendum(origin, #[compact] ref_index: ReferendumIndex) { ensure_root(origin)?; Self::internal_cancel_referendum(ref_index); @@ -827,22 +902,23 @@ decl_module! { /// - `which`: The index of the referendum to cancel. /// /// # - /// - One DB change. - /// - O(d) where d is the items in the dispatch queue. + /// - Db reads: `scheduler lookup`, scheduler agenda` + /// - Db writes: `scheduler lookup`, scheduler agenda` + /// - O(D) where D is the items in the dispatch queue. Weighted as D=10. /// # - #[weight = (0, DispatchClass::Operational)] + #[weight = (50_000_000 + T::DbWeight::get().reads_writes(2, 2), DispatchClass::Operational)] fn cancel_queued(origin, which: ReferendumIndex) { ensure_root(origin)?; T::Scheduler::cancel_named((DEMOCRACY_ID, which)) .map_err(|_| Error::::ProposalMissing)?; } + /// Weight: see `begin_block` fn on_initialize(n: T::BlockNumber) -> Weight { - if let Err(e) = Self::begin_block(n) { + Self::begin_block(n).unwrap_or_else(|e| { sp_runtime::print(e); - } - - 0 + 0 + }) } /// Specify a proxy that is already open to us. Called by the stash. @@ -854,9 +930,11 @@ decl_module! { /// - `proxy`: The account that will be activated as proxy. /// /// # - /// - One extra DB entry. + /// - Complexity: `O(1)` + /// - Db reads: `Proxy` + /// - Db writes: `Proxy` /// # - #[weight = 100_000_000] + #[weight = 11_000_000 + T::DbWeight::get().reads_writes(1, 1)] fn activate_proxy(origin, proxy: T::AccountId) { let who = ensure_signed(origin)?; Proxy::::try_mutate(&proxy, |a| match a.take() { @@ -877,9 +955,11 @@ decl_module! { /// The dispatch origin of this call must be _Signed_. /// /// # - /// - One DB clear. + /// - Complexity: `O(1)` + /// - Db reads: `Proxy`, `sender account` + /// - Db writes: `Proxy`, `sender account` /// # - #[weight = 100_000_000] + #[weight = 21_000_000 + T::DbWeight::get().reads_writes(1, 1)] fn close_proxy(origin) { let who = ensure_signed(origin)?; Proxy::::mutate(&who, |a| { @@ -901,9 +981,11 @@ decl_module! { /// - `proxy`: The account that will be deactivated as proxy. /// /// # - /// - One DB clear. + /// - Complexity: `O(1)` + /// - Db reads: `Proxy` + /// - Db writes: `Proxy` /// # - #[weight = 100_000_000] + #[weight = 11_000_000 + T::DbWeight::get().reads_writes(1, 1)] fn deactivate_proxy(origin, proxy: T::AccountId) { let who = ensure_signed(origin)?; Proxy::::try_mutate(&proxy, |a| match a.take() { @@ -935,8 +1017,17 @@ decl_module! { /// Emits `Delegated`. /// /// # + /// - Complexity: `O(R)` where R is the number of referendums the voter delegating to has + /// voted on. Weight is charged as if maximum votes. + /// - Db reads: 2*`VotingOf`, `balances locks` + /// - Db writes: 2*`VotingOf`, `balances locks` + /// - Db reads per votes: `ReferendumInfoOf` + /// - Db writes per votes: `ReferendumInfoOf` + // NOTE: weight must cover an incorrect voting of origin with 100 votes. /// # - #[weight = 500_000_000] + #[weight = 82_000_000 + 8_800_000 * Weight::from(T::MaxVotes::get()) + + T::DbWeight::get().reads_writes(3 + Weight::from(T::MaxVotes::get()), 3 + Weight::from(T::MaxVotes::get()))] + // TODO TODO: this might be too expensive, at least we should refund maybe. pub fn delegate(origin, to: T::AccountId, conviction: Conviction, balance: BalanceOf) { let who = ensure_signed(origin)?; Self::try_delegate(who, to, conviction, balance)?; @@ -953,9 +1044,17 @@ decl_module! { /// Emits `Undelegated`. /// /// # - /// - O(1). + /// - Complexity: `O(R)` where R is the number of referendums the voter delegating to has + /// voted on. Weight is charged as if maximum votes. + /// - Db reads: 2*`VotingOf` + /// - Db writes: 2*`VotingOf` + /// - Db reads per votes: `ReferendumInfoOf` + /// - Db writes per votes: `ReferendumInfoOf` + // NOTE: weight must cover an incorrect voting of origin with 100 votes. /// # - #[weight = 500_000_000] + #[weight = 42_000_000 + 8_800_000 * Weight::from(T::MaxVotes::get()) + + T::DbWeight::get().reads_writes(2 + Weight::from(T::MaxVotes::get()), 2 + Weight::from(T::MaxVotes::get()))] + // TODO TODO: this might be too expensive, at least we should refund maybe. fn undelegate(origin) { let who = ensure_signed(origin)?; Self::try_undelegate(who)?; @@ -967,9 +1066,9 @@ decl_module! { /// /// # /// - `O(1)`. - /// - One DB clear. + /// - Db writes: `PublicProps` /// # - #[weight = 0] + #[weight = 4_000_000 + T::DbWeight::get().writes(1)] fn clear_public_proposals(origin) { ensure_root(origin)?; @@ -986,10 +1085,12 @@ decl_module! { /// Emits `PreimageNoted`. /// /// # - /// - Dependent on the size of `encoded_proposal` but protected by a - /// required deposit. + /// - Complexity: `O(E)` with E size of `encoded_proposal` (protected by a required deposit). + /// - Db reads: `Preimages` + /// - Db writes: `Preimages` /// # - #[weight = 100_000_000] + #[weight = 43_000_000 + 4_000 * Weight::from(encoded_proposal.len() as u32) + + T::DbWeight::get().reads_writes(1, 1)] fn note_preimage(origin, encoded_proposal: Vec) { let who = ensure_signed(origin)?; let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); @@ -1022,12 +1123,16 @@ decl_module! { /// Emits `PreimageNoted`. /// /// # - /// - Dependent on the size of `encoded_proposal` and length of dispatch queue. + /// - Complexity: `O(E)` with E size of `encoded_proposal` (protected by a required deposit). + /// - Db reads: `Preimages` + /// - Db writes: `Preimages` /// # - #[weight = 100_000_000] + #[weight = 31_000_000 + 4_000 * Weight::from(encoded_proposal.len() as u32) + + T::DbWeight::get().reads_writes(1, 1)] fn note_imminent_preimage(origin, encoded_proposal: Vec) { let who = ensure_signed(origin)?; let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); + Self::check_pre_image_is_missing(proposal_hash)?; let status = Preimages::::get(&proposal_hash).ok_or(Error::::NotImminent)?; let expiry = status.to_missing_expiry().ok_or(Error::::DuplicatePreimage)?; @@ -1050,6 +1155,8 @@ decl_module! { /// The dispatch origin of this call must be _Signed_. /// /// - `proposal_hash`: The preimage hash of a proposal. + /// - `proposal_length_upper_bound`: an upper bound on length of the proposal. + /// Extrinsic is weighted according to this value with no refund. /// /// This will only work after `VotingPeriod` blocks from the time that the preimage was /// noted, if it's the same account doing it. If it's a different account, then it'll only @@ -1058,11 +1165,20 @@ decl_module! { /// Emits `PreimageReaped`. /// /// # - /// - One DB clear. + /// - Complexity: `O(D)` where D is length of proposal. + /// - Db reads: `Preimages` + /// - Db writes: `Preimages` /// # - #[weight = 0] - fn reap_preimage(origin, proposal_hash: T::Hash) { + #[weight = (47_000_000 + T::DbWeight::get().reads_writes(1, 1)) + .saturating_add(3_000.saturating_mul(Weight::from(*proposal_len_upper_bound)))] + fn reap_preimage(origin, proposal_hash: T::Hash, proposal_len_upper_bound: u32) { let who = ensure_signed(origin)?; + + ensure!( + Self::pre_image_data_len(proposal_hash)? <= proposal_len_upper_bound, + Error::::WrongUpperBound, + ); + let (provider, deposit, since, expiry) = >::get(&proposal_hash) .and_then(|m| match m { PreimageStatus::Available { provider, deposit, since, expiry, .. } @@ -1088,9 +1204,12 @@ decl_module! { /// - `target`: The account to remove the lock on. /// /// # - /// - `O(1)`. + /// - Complexity `O(R)` with R number of vote of target. + /// - Db reads: `VotingOf`, `balances locks`, `target account` + /// - Db writes: `VotingOf`, `balances locks`, `target account` /// # - #[weight = 0] + #[weight = 53_000_000 + 220_000 * Weight::from(T::MaxVotes::get()) + + T::DbWeight::get().reads_writes(3, 3)] fn unlock(origin, target: T::AccountId) { ensure_signed(origin)?; Self::update_lock(&target); @@ -1107,9 +1226,11 @@ decl_module! { /// `close_proxy` must be called before the account can be destroyed. /// /// # - /// - One extra DB entry. + /// - Complexity: O(1) + /// - Db reads: `Proxy`, `proxy account` + /// - Db writes: `Proxy`, `proxy account` /// # - #[weight = 100_000_000] + #[weight = 21_000_000 + T::DbWeight::get().reads_writes(2, 2)] fn open_proxy(origin, target: T::AccountId) { let who = ensure_signed(origin)?; Proxy::::mutate(&who, |a| { @@ -1147,8 +1268,11 @@ decl_module! { /// /// # /// - `O(R + log R)` where R is the number of referenda that `target` has voted on. + /// Weight is calculated for the maximum number of vote. + /// - Db reads: `ReferendumInfoOf`, `VotingOf` + /// - Db writes: `ReferendumInfoOf`, `VotingOf` /// # - #[weight = 0] + #[weight = 30_000_000 + 210_000 * Weight::from(T::MaxVotes::get()) + T::DbWeight::get().reads_writes(2, 2)] fn remove_vote(origin, index: ReferendumIndex) -> DispatchResult { let who = ensure_signed(origin)?; Self::try_remove_vote(&who, index, UnvoteScope::Any) @@ -1169,8 +1293,11 @@ decl_module! { /// /// # /// - `O(R + log R)` where R is the number of referenda that `target` has voted on. + /// Weight is calculated for the maximum number of vote. + /// - Db reads: `ReferendumInfoOf`, `VotingOf` + /// - Db writes: `ReferendumInfoOf`, `VotingOf` /// # - #[weight = 0] + #[weight = 30_000_000 + 210_000 * Weight::from(T::MaxVotes::get()) + T::DbWeight::get().reads_writes(2, 2)] fn remove_other_vote(origin, target: T::AccountId, index: ReferendumIndex) -> DispatchResult { let who = ensure_signed(origin)?; let scope = if target == who { UnvoteScope::Any } else { UnvoteScope::OnlyExpired }; @@ -1200,8 +1327,13 @@ decl_module! { /// Emits `Delegated`. /// /// # + /// same as `delegate with additional: + /// Db reads: `Proxy`, `proxy account` + /// Db writes: `proxy account` /// # - #[weight = 500_000_000] + #[weight = 92_000_000 + 9_100_000 * Weight::from(T::MaxVotes::get()) + + T::DbWeight::get().reads_writes(5 + Weight::from(T::MaxVotes::get()), 4 + Weight::from(T::MaxVotes::get()))] + // TODO TODO: this might be too expensive, at least we should refund maybe. pub fn proxy_delegate(origin, to: T::AccountId, conviction: Conviction, @@ -1223,9 +1355,12 @@ decl_module! { /// Emits `Undelegated`. /// /// # - /// - O(1). + /// same as `undelegate with additional: + /// Db reads: `Proxy` /// # - #[weight = 500_000_000] + #[weight = 50_000_000 + 9_900_000 * Weight::from(T::MaxVotes::get()) + + T::DbWeight::get().reads_writes(3 + Weight::from(T::MaxVotes::get()), 2 + Weight::from(T::MaxVotes::get()))] + // TODO TODO: this might be too expensive, at least we should refund maybe. fn proxy_undelegate(origin) { let who = ensure_signed(origin)?; let target = Self::proxy(who).and_then(|a| a.as_active()).ok_or(Error::::NotProxy)?; @@ -1244,8 +1379,11 @@ decl_module! { /// /// # /// - `O(R + log R)` where R is the number of referenda that `target` has voted on. + /// Weight is calculated for the maximum number of vote. + /// - Db reads: `ReferendumInfoOf`, `VotingOf`, `Proxy` + /// - Db writes: `ReferendumInfoOf`, `VotingOf` /// # - #[weight = 0] + #[weight = 38_000_000 + 210_000 * Weight::from(T::MaxVotes::get()) + T::DbWeight::get().reads_writes(2, 3)] fn proxy_remove_vote(origin, index: ReferendumIndex) -> DispatchResult { let who = ensure_signed(origin)?; let target = Self::proxy(who).and_then(|a| a.as_active()).ok_or(Error::::NotProxy)?; @@ -1267,7 +1405,7 @@ impl Module { /// Get the amount locked in support of `proposal`; `None` if proposal isn't a valid proposal /// index. pub fn backing_for(proposal: PropIndex) -> Option> { - Self::deposit_of(proposal).map(|(d, l)| d * (l.len() as u32).into()) + Self::deposit_of(proposal).map(|(l, d)| d * (l.len() as u32).into()) } /// Get all referenda ready for tally at block `n`. @@ -1353,7 +1491,10 @@ impl Module { } votes[i].1 = vote; } - Err(i) => votes.insert(i, (ref_index, vote)), + Err(i) => { + ensure!(votes.len() as u32 <= T::MaxVotes::get(), Error::::MaxVotesReached); + votes.insert(i, (ref_index, vote)); + } } // Shouldn't be possible to fail, but we handle it gracefully. status.tally.add(vote).ok_or(Error::::Overflow)?; @@ -1598,7 +1739,7 @@ impl Module { let (prop_index, proposal, _) = public_props.swap_remove(winner_index); >::put(public_props); - if let Some((deposit, depositors)) = >::take(prop_index) { + if let Some((depositors, deposit)) = >::take(prop_index) { // refund depositors for d in &depositors { T::Currency::unreserve(d, deposit); @@ -1677,19 +1818,102 @@ impl Module { } /// Current era is ending; we should finish up any proposals. - fn begin_block(now: T::BlockNumber) -> DispatchResult { + /// + /// If a referendum is launched or maturing take full block weight. + fn begin_block(now: T::BlockNumber) -> Result { + // TODO TODO: do idle weight using benchmark on_initialize_no_launch_no_maturing + let mut weight = 0; // pick out another public referendum if it's time. if (now % T::LaunchPeriod::get()).is_zero() { // Errors come from the queue being empty. we don't really care about that, and even if // we did, there is nothing we can do here. let _ = Self::launch_next(now); + weight = T::MaximumBlockWeight::get(); } // tally up votes for any expiring referenda. for (index, info) in Self::maturing_referenda_at(now).into_iter() { let approved = Self::bake_referendum(now, index, info)?; ReferendumInfoOf::::insert(index, ReferendumInfo::Finished { end: now, approved }); + weight = T::MaximumBlockWeight::get(); + } + + Ok(weight) + } + + /// Reads the length of PublicProps without getting the complete value in the runtime. + fn len_of_public_props() -> u32 { + decode_compact_u32_at(&>::hashed_key()).unwrap_or(0) + } + + /// Reads the length of account in DepositOf without getting the complete value in the runtime. + /// + /// Return 0 if no deposit for this proposal. + fn len_of_deposit_of(proposal: PropIndex) -> Option { + decode_compact_u32_at(&>::hashed_key_for(proposal)) + } + + /// Check pre image is missing variant without getting the complete value in the runtime. + fn check_pre_image_is_missing(proposal_hash: T::Hash) -> DispatchResult { + let mut buf = [0u8; 1]; + let key = >::hashed_key_for(proposal_hash); + let bytes = match sp_io::storage::read(&key, &mut buf, 0) { + Some(bytes) => bytes, + None => return Err(Error::::NotImminent.into()), + }; + let mut input = &buf[0..buf.len().min(bytes as usize)]; + + match input.read_byte() { + Ok(0) => Ok(()), + Ok(1) => Err(Error::::DuplicatePreimage.into()), + _ => { + sp_runtime::print("Failed to decode `PreimageStatus` variant"); + Err(Error::::NotImminent.into()) + } + } + } + + /// Check pre image is missing variant without getting the complete value in the runtime. + fn pre_image_data_len(proposal_hash: T::Hash) -> Result { + let mut buf = [0u8; 6]; + let key = >::hashed_key_for(proposal_hash); + let bytes = match sp_io::storage::read(&key, &mut buf, 0) { + Some(bytes) => bytes, + None => return Err(Error::::PreimageMissing.into()), + }; + let mut input = &buf[0..buf.len().min(bytes as usize)]; + + match input.read_byte() { + Ok(1) => (), + Ok(0) => return Err(Error::::PreimageMissing.into()), + _ => { + sp_runtime::print("Failed to decode `PreimageStatus` variant"); + return Err(Error::::PreimageMissing.into()); + } + } + + let len = codec::Compact::::decode(&mut input).map_err(|_| { + sp_runtime::print("Failed to decode `PreimageStatus` variant"); + DispatchError::from(Error::::PreimageMissing) + })?.0; + + Ok(len) + } +} + +fn decode_compact_u32_at(key: &[u8]) -> Option { + let mut buf = [0u8; 5]; + let bytes = match sp_io::storage::read(&key, &mut buf, 0) { + Some(bytes) => bytes, + None => return None, + }; + let mut input = &buf[0..buf.len().min(bytes as usize)]; + match codec::Compact::::decode(&mut input) { + Ok(c) => Some(c.0), + Err(_) => { + sp_runtime::print("Failed to decode compact u32 at:"); + sp_runtime::print(key); + None } - Ok(()) } } diff --git a/frame/democracy/src/tests.rs b/frame/democracy/src/tests.rs index 076eb66d6bb56..b2c51bc8ebd36 100644 --- a/frame/democracy/src/tests.rs +++ b/frame/democracy/src/tests.rs @@ -41,6 +41,8 @@ mod proxying; mod public_proposals; mod scheduling; mod voting; +mod migration; +mod decoders; const AYE: Vote = Vote { aye: true, conviction: Conviction::None }; const NAY: Vote = Vote { aye: false, conviction: Conviction::None }; @@ -129,6 +131,8 @@ parameter_types! { pub const MinimumDeposit: u64 = 1; pub const EnactmentPeriod: u64 = 2; pub const CooloffPeriod: u64 = 2; + pub const MaxVetoers: u32 = 10; + pub const MaxVotes: u32 = 100; } ord_parameter_types! { pub const One: u64 = 1; @@ -179,6 +183,8 @@ impl super::Trait for Test { type InstantOrigin = EnsureSignedBy; type InstantAllowed = InstantAllowed; type Scheduler = Scheduler; + type MaxVetoers = MaxVetoers; + type MaxVotes = MaxVotes; } pub fn new_test_ext() -> sp_io::TestExternalities { @@ -229,7 +235,8 @@ fn propose_set_balance(who: u64, value: u64, delay: u64) -> DispatchResult { Democracy::propose( Origin::signed(who), set_balance_proposal_hash(value), - delay + delay, + u32::max_value(), ) } @@ -237,14 +244,15 @@ fn propose_set_balance_and_note(who: u64, value: u64, delay: u64) -> DispatchRes Democracy::propose( Origin::signed(who), set_balance_proposal_hash_and_note(value), - delay + delay, + u32::max_value(), ) } fn next_block() { System::set_block_number(System::block_number() + 1); Scheduler::on_initialize(System::block_number()); - assert_eq!(Democracy::begin_block(System::block_number()), Ok(())); + assert!(Democracy::begin_block(System::block_number()).is_ok()); } fn fast_forward_to(n: u64) { diff --git a/frame/democracy/src/tests/decoders.rs b/frame/democracy/src/tests/decoders.rs new file mode 100644 index 0000000000000..4d07b7c075cd0 --- /dev/null +++ b/frame/democracy/src/tests/decoders.rs @@ -0,0 +1,75 @@ +// Copyright 2017-2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! The for various partial storage decoders + +use super::*; +use frame_support::{storage::migration, storage::{StorageMap, StorageValue}}; + +#[test] +fn len_of_public_props() { + new_test_ext().execute_with(|| { + let value: Vec<(PropIndex, H256, u64)> = (0..10).map(|_| Default::default()).collect(); + PublicProps::::put(value); + assert_eq!(Democracy::len_of_public_props(), 10); + + PublicProps::::kill(); + assert_eq!(Democracy::len_of_public_props(), 0); + + let v = codec::Compact(u64::max_value()); + migration::put_storage_value(b"Democracy", b"PublicProps", &[], v); + assert_eq!(Democracy::len_of_public_props(), 0); + }) +} + +#[test] +fn len_of_deposit_of() { + new_test_ext().execute_with(|| { + let value: (Vec, u64) = ((0..3).map(|_| Default::default()).collect(), 3u64); + DepositOf::::insert(2, value); + assert_eq!(Democracy::len_of_deposit_of(2), Some(3)); + + DepositOf::::remove(2); + assert_eq!(Democracy::len_of_deposit_of(2), None); + }) +} + +#[test] +fn pre_image() { + new_test_ext().execute_with(|| { + let key = Default::default(); + let missing = PreimageStatus::Missing(0); + let available = PreimageStatus::Available{ + data: (0..10u8).collect(), + provider: 0, + deposit: 0, + since: 0, + expiry: None, + }; + + Preimages::::insert(key, missing); + assert!(Democracy::pre_image_data_len(key).is_err()); + assert_eq!(Democracy::check_pre_image_is_missing(key), Ok(())); + + Preimages::::remove(key); + assert!(Democracy::pre_image_data_len(key).is_err()); + assert!(Democracy::check_pre_image_is_missing(key).is_err()); + + Preimages::::insert(key, available); + assert_eq!(Democracy::pre_image_data_len(key), Ok(10)); + assert!(Democracy::check_pre_image_is_missing(key).is_err()); + }) +} diff --git a/frame/democracy/src/tests/migration.rs b/frame/democracy/src/tests/migration.rs new file mode 100644 index 0000000000000..f82540d9c69f7 --- /dev/null +++ b/frame/democracy/src/tests/migration.rs @@ -0,0 +1,45 @@ +// Copyright 2017-2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! The tests for migration. + +use super::*; +use frame_support::{storage::migration, Hashable, traits::OnRuntimeUpgrade}; +use substrate_test_utils::assert_eq_uvec; + +#[test] +fn migration() { + new_test_ext().execute_with(|| { + for i in 0..3 { + let k = i.twox_64_concat(); + let v: (BalanceOf, Vec) = (i * 1000, vec![i]); + migration::put_storage_value(b"Democracy", b"DepositOf", &k, v); + } + StorageVersion::put(Releases::V1); + + Democracy::on_runtime_upgrade(); + + assert_eq!(StorageVersion::get(), Releases::V2); + assert_eq_uvec!( + DepositOf::::iter().collect::>(), + vec![ + (0, (vec![0u64], >::from(0u32))), + (1, (vec![1u64], >::from(1000u32))), + (2, (vec![2u64], >::from(2000u32))), + ] + ); + }) +} diff --git a/frame/democracy/src/tests/preimage.rs b/frame/democracy/src/tests/preimage.rs index 7d977b0ba83ac..ffa8b46790e13 100644 --- a/frame/democracy/src/tests/preimage.rs +++ b/frame/democracy/src/tests/preimage.rs @@ -76,11 +76,11 @@ fn preimage_deposit_should_be_reapable_earlier_by_owner() { next_block(); assert_noop!( - Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2)), + Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2), u32::max_value()), Error::::TooEarly ); next_block(); - assert_ok!(Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2))); + assert_ok!(Democracy::reap_preimage(Origin::signed(6), set_balance_proposal_hash(2), u32::max_value())); assert_eq!(Balances::free_balance(6), 60); assert_eq!(Balances::reserved_balance(6), 0); @@ -91,7 +91,7 @@ fn preimage_deposit_should_be_reapable_earlier_by_owner() { fn preimage_deposit_should_be_reapable() { new_test_ext().execute_with(|| { assert_noop!( - Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2)), + Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2), u32::max_value()), Error::::PreimageMissing ); @@ -103,12 +103,12 @@ fn preimage_deposit_should_be_reapable() { next_block(); next_block(); assert_noop!( - Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2)), + Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2), u32::max_value()), Error::::TooEarly ); next_block(); - assert_ok!(Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2))); + assert_ok!(Democracy::reap_preimage(Origin::signed(5), set_balance_proposal_hash(2), u32::max_value())); assert_eq!(Balances::reserved_balance(6), 0); assert_eq!(Balances::free_balance(6), 48); assert_eq!(Balances::free_balance(5), 62); @@ -152,6 +152,6 @@ fn reaping_imminent_preimage_should_fail() { assert_ok!(Democracy::vote(Origin::signed(1), r, aye(1))); next_block(); next_block(); - assert_noop!(Democracy::reap_preimage(Origin::signed(6), h), Error::::Imminent); + assert_noop!(Democracy::reap_preimage(Origin::signed(6), h, u32::max_value()), Error::::Imminent); }); } diff --git a/frame/democracy/src/tests/public_proposals.rs b/frame/democracy/src/tests/public_proposals.rs index 04246e86f1da4..ef7aa441f95e6 100644 --- a/frame/democracy/src/tests/public_proposals.rs +++ b/frame/democracy/src/tests/public_proposals.rs @@ -34,10 +34,10 @@ fn backing_for_should_work() { fn deposit_for_proposals_should_be_taken() { new_test_ext().execute_with(|| { assert_ok!(propose_set_balance_and_note(1, 2, 5)); - assert_ok!(Democracy::second(Origin::signed(2), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); + assert_ok!(Democracy::second(Origin::signed(2), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::free_balance(2), 15); assert_eq!(Balances::free_balance(5), 35); @@ -48,10 +48,10 @@ fn deposit_for_proposals_should_be_taken() { fn deposit_for_proposals_should_be_returned() { new_test_ext().execute_with(|| { assert_ok!(propose_set_balance_and_note(1, 2, 5)); - assert_ok!(Democracy::second(Origin::signed(2), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); - assert_ok!(Democracy::second(Origin::signed(5), 0)); + assert_ok!(Democracy::second(Origin::signed(2), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); + assert_ok!(Democracy::second(Origin::signed(5), 0, u32::max_value())); fast_forward_to(3); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::free_balance(2), 20); @@ -77,7 +77,10 @@ fn poor_proposer_should_not_work() { fn poor_seconder_should_not_work() { new_test_ext().execute_with(|| { assert_ok!(propose_set_balance_and_note(2, 2, 11)); - assert_noop!(Democracy::second(Origin::signed(1), 0), BalancesError::::InsufficientBalance); + assert_noop!( + Democracy::second(Origin::signed(1), 0, u32::max_value()), + BalancesError::::InsufficientBalance + ); }); }