Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions frame/elections-phragmen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ use frame_support::{
traits::{
Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons,
ChangeMembers, OnUnbalanced, WithdrawReason, Contains, BalanceStatus, InitializeMembers,
ContainsCountUpperBound,
}
};
use sp_phragmen::{build_support_map, ExtendedBalance, VoteWeight, PhragmenResult};
Expand Down Expand Up @@ -878,6 +879,13 @@ impl<T: Trait> Contains<T::AccountId> for Module<T> {
}
}

/// Implementation uses a parameter type so calling is cost-free.
impl<T: Trait> ContainsCountUpperBound for Module<T> {
fn count_upper_bound() -> usize {
Comment thread
gui1117 marked this conversation as resolved.
Outdated
Self::desired_members() as usize
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
11 changes: 8 additions & 3 deletions frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,7 @@ impl<T: Default> Get<T> for () {
fn get() -> T { T::default() }
}

/// A trait for querying whether a type can be said to statically "contain" a value. Similar
/// in nature to `Get`, except it is designed to be lazy rather than active (you can't ask it to
/// enumerate all values that it contains) and work for multiple values rather than just one.
/// A trait for querying whether a type can be said to "contain" a value.
Comment thread
apopiak marked this conversation as resolved.
pub trait Contains<T: Ord> {
/// Return `true` if this "contains" the given value `t`.
fn contains(t: &T) -> bool { Self::sorted_members().binary_search(t).is_ok() }
Expand All @@ -211,6 +209,13 @@ pub trait Contains<T: Ord> {
fn add(t: &T) { unimplemented!() }
}

/// A trait for querying an upper bound on the number of element in a type that implements
/// `Contains`.
pub trait ContainsCountUpperBound {
Comment thread
gui1117 marked this conversation as resolved.
Outdated
/// An upper bound for the number of element contained.
Comment thread
gui1117 marked this conversation as resolved.
Outdated
fn count_upper_bound() -> usize;
}

/// Determiner to say whether a given account is unused.
pub trait IsDeadAccount<AccountId> {
/// Is the given account dead?
Expand Down
122 changes: 78 additions & 44 deletions frame/treasury/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ use frame_support::traits::{
use sp_runtime::{Permill, ModuleId, Percent, RuntimeDebug, traits::{
Zero, StaticLookup, AccountIdConversion, Saturating, Hash, BadOrigin
}};
use frame_support::weights::{Weight, MINIMUM_WEIGHT, DispatchClass};
use frame_support::traits::{Contains, EnsureOrigin};
use frame_support::weights::Weight;
use frame_support::traits::{Contains, ContainsCountUpperBound, EnsureOrigin};
use codec::{Encode, Decode};
use frame_system::{self as system, ensure_signed, ensure_root};

Expand All @@ -124,7 +124,10 @@ pub trait Trait: frame_system::Trait {
type RejectOrigin: EnsureOrigin<Self::Origin>;

/// Origin from which tippers must come.
type Tippers: Contains<Self::AccountId>;
///
/// `ContainsCountUpperBound::count_upper_bound` must be cost free.
/// (i.e. no storage read or heavy operation)
type Tippers: Contains<Self::AccountId> + ContainsCountUpperBound;

/// The period for which a tip remains open after is has achieved threshold tippers.
type TipCountdown: Get<Self::BlockNumber>;
Expand Down Expand Up @@ -323,11 +326,11 @@ decl_module! {
/// proposal is awarded.
///
/// # <weight>
/// - O(1).
/// - Limited storage reads.
/// - One DB change, one extra DB entry.
/// - Complexity: O(1)
/// - DbReads: `ProposalCount`, `sender account`
/// - DbWrites: `ProposalCount`, `Proposals`, `sender account`
/// # </weight>
#[weight = 500_000_000]
#[weight = 114_700_000 + T::DbWeight::get().reads_writes(1, 2)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so can you bring me up to speed, where are these numbers coming from? the 114_700_000 per se.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • to me it seems like we ignore sender account all the time here. Correct? Also best to call it origin in the doc as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this number directly comes from the benchmark server at https://www.shawntabrizi.com/substrate-graph-benchmarks/?p=treasury&e=tip_new

fn propose_spend(
origin,
#[compact] value: BalanceOf<T>,
Expand All @@ -350,11 +353,11 @@ decl_module! {
/// Reject a proposed spend. The original deposit will be slashed.
///
/// # <weight>
/// - O(1).
/// - Limited storage reads.
/// - One DB clear.
/// - Complexity: O(1)
/// - DbReads: `Proposals`, `rejected proposer account`
/// - DbWrites: `Proposals`, `rejected proposer account`
/// # </weight>
#[weight = (100_000_000, DispatchClass::Operational)]
#[weight = 125_800_000 + T::DbWeight::get().reads_writes(2, 2)]
fn reject_proposal(origin, #[compact] proposal_id: ProposalIndex) {
T::RejectOrigin::try_origin(origin)
.map(|_| ())
Expand All @@ -372,11 +375,11 @@ decl_module! {
/// and the original deposit will be returned.
///
/// # <weight>
/// - O(1).
/// - Limited storage reads.
/// - One DB change.
/// - Complexity: O(1).
/// - DbReads: `Proposals`, `Approvals`
/// - DbWrite: `Approvals`
/// # </weight>
#[weight = (100_000_000, DispatchClass::Operational)]
#[weight = 33_610_000 + T::DbWeight::get().reads_writes(2, 1)]
Comment thread
apopiak marked this conversation as resolved.
Outdated
fn approve_proposal(origin, #[compact] proposal_id: ProposalIndex) {
T::ApproveOrigin::try_origin(origin)
.map(|_| ())
Expand All @@ -400,12 +403,12 @@ decl_module! {
/// Emits `NewTip` if successful.
///
/// # <weight>
/// - `O(R)` where `R` length of `reason`.
/// - One balance operation.
/// - One storage mutation (codec `O(R)`).
/// - One event.
/// - Complexity: `O(R)` where `R` length of `reason`.
/// - encoding and hashing of 'reason'
/// - DbReads: `Reasons`, `Tips`, `who account data`
/// - DbWrites: `Tips`, `who account data`
/// # </weight>
#[weight = 100_000_000]
#[weight = 138_400_000 + 4_000 * reason.len() as u64 + T::DbWeight::get().reads_writes(3, 2)]
Comment thread
apopiak marked this conversation as resolved.
Outdated
Comment thread
gui1117 marked this conversation as resolved.
Outdated
fn report_awesome(origin, reason: Vec<u8>, who: T::AccountId) {
let finder = ensure_signed(origin)?;

Expand Down Expand Up @@ -442,12 +445,12 @@ decl_module! {
/// Emits `TipRetracted` if successful.
///
/// # <weight>
/// - `O(T)`
/// - One balance operation.
/// - Two storage removals (one read, codec `O(T)`).
/// - One event.
/// - Complexity: `O(1)`
/// - Depends on the length of `T::Hash` which is fixed.
/// - DbReads: `Tips`, `sender account data`
/// - DbWrites: `Reasons`, `Tips`, `sender account data`
/// # </weight>
#[weight = 50_000_000]
#[weight = 115_700_000 + T::DbWeight::get().reads_writes(1, 2)]
Comment thread
apopiak marked this conversation as resolved.
Outdated
fn retract_tip(origin, hash: T::Hash) {
let who = ensure_signed(origin)?;
let tip = Tips::<T>::get(&hash).ok_or(Error::<T>::UnknownTip)?;
Expand All @@ -474,12 +477,18 @@ decl_module! {
/// Emits `NewTip` if successful.
///
/// # <weight>
/// - `O(R + T)` where `R` length of `reason`, `T` is the number of tippers. `T` is
/// naturally capped as a membership set, `R` is limited through transaction-size.
/// - Two storage insertions (codecs `O(R)`, `O(T)`), one read `O(1)`.
/// - One event.
/// - Complexity: `O(R + T)` where `R` length of `reason`, `T` is the number of tippers.
/// - `O(T)`: decoding `Tipper` vec of length `T`
/// `T` is charged as upper bound given by `ContainsCountUpperBound`.
/// The actual cost depends on the implementation of `T::Tippers`.
/// - `O(R)`: hashing and encoding of reason of length `R`
/// - DbReads: `Tippers`, `Reasons`
/// - DbWrites: `Reasons`, `Tips`
/// # </weight>
#[weight = 150_000_000]
#[weight = 110_000_000
+ 4_000 * reason.len() as u64
Comment thread
apopiak marked this conversation as resolved.
Outdated
Comment thread
gui1117 marked this conversation as resolved.
Outdated
+ 480_000 * T::Tippers::count_upper_bound() as u64
+ T::DbWeight::get().reads_writes(2, 2)]
fn tip_new(origin, reason: Vec<u8>, who: T::AccountId, tip_value: BalanceOf<T>) {
let tipper = ensure_signed(origin)?;
ensure!(T::Tippers::contains(&tipper), BadOrigin);
Expand Down Expand Up @@ -509,11 +518,18 @@ decl_module! {
/// has started.
///
/// # <weight>
/// - `O(T)`
/// - One storage mutation (codec `O(T)`), one storage read `O(1)`.
/// - Up to one event.
/// - Complexity: `O(T)` where `T` is the number of tippers.
/// decoding `Tipper` vec of length `T`, insert tip and check closing,
/// `T` is charged as upper bound given by `ContainsCountUpperBound`.
/// The actual cost depends on the implementation of `T::Tippers`.
///
/// Actually weight could be lower as it depends on how many tips are in `OpenTip` but it
/// is weighted as if almost full i.e of length `T-1`.
/// - DbReads: `Tippers`, `Tips`
/// - DbWrites: `Tips`
/// # </weight>
#[weight = 50_000_000]
#[weight = 68_000_000 + 1_900_000 * T::Tippers::count_upper_bound() as u64
Comment thread
gui1117 marked this conversation as resolved.
Outdated
+ T::DbWeight::get().reads_writes(2, 1)]
fn tip(origin, hash: T::Hash, tip_value: BalanceOf<T>) {
let tipper = ensure_signed(origin)?;
ensure!(T::Tippers::contains(&tipper), BadOrigin);
Expand All @@ -535,11 +551,15 @@ decl_module! {
/// as the hash of the tuple of the original tip `reason` and the beneficiary account ID.
///
/// # <weight>
/// - `O(T)`
/// - One storage retrieval (codec `O(T)`) and two removals.
/// - Up to three balance operations.
/// - Complexity: `O(T)` where `T` is the number of tippers.
/// decoding `Tipper` vec of length `T`.
/// `T` is charged as upper bound given by `ContainsCountUpperBound`.
/// The actual cost depends on the implementation of `T::Tippers`.
/// - DbReads: `Tips`, `Tippers`, `tip finder`
/// - DbWrites: `Reasons`, `Tips`, `Tippers`, `tip finder`
/// # </weight>
#[weight = 50_000_000]
#[weight = 210_000_000 + 1_100_000 * T::Tippers::count_upper_bound() as u64
Comment thread
gui1117 marked this conversation as resolved.
Outdated
+ T::DbWeight::get().reads_writes(3, 3)]
fn close_tip(origin, hash: T::Hash) {
ensure_signed(origin)?;

Expand All @@ -552,13 +572,23 @@ decl_module! {
Self::payout_tip(hash, tip);
}

/// # <weight>
/// - Complexity: `O(A)` where `A` is the number of approvals
/// - Db reads and writes: `Approvals`, `pot account data`
/// - Db reads and writes per approval:
/// `Proposals`, `proposer account data`, `beneficiary account data`
/// - The weight is overestimated if some approvals got missed.
/// # </weight>
fn on_initialize(n: T::BlockNumber) -> Weight {
// Check to see if we should spend some funds!
if (n % T::SpendPeriod::get()).is_zero() {
Self::spend_funds();
}
let approvals_len = Self::spend_funds();

MINIMUM_WEIGHT
260_000_000 * approvals_len
Comment thread
gui1117 marked this conversation as resolved.
Outdated
+ T::DbWeight::get().reads_writes(2 + approvals_len * 3, 2 + approvals_len * 3)
} else {
0
}
}
}
}
Expand Down Expand Up @@ -650,14 +680,15 @@ impl<T: Trait> Module<T> {
Self::deposit_event(RawEvent::TipClosed(hash, tip.who, payout));
}

// Spend some money!
fn spend_funds() {
/// Spend some money! returns number of approvals before spend.
fn spend_funds() -> u64 {
let mut budget_remaining = Self::pot();
Self::deposit_event(RawEvent::Spending(budget_remaining));

let mut missed_any = false;
let mut imbalance = <PositiveImbalanceOf<T>>::zero();
Approvals::mutate(|v| {
let prior_approvals_len = Approvals::mutate(|v| {
let prior_approvals_len = v.len() as u64;
v.retain(|&index| {
// Should always be true, but shouldn't panic if false or we're screwed.
if let Some(p) = Self::proposals(index) {
Expand All @@ -681,6 +712,7 @@ impl<T: Trait> Module<T> {
false
}
});
prior_approvals_len
});

if !missed_any {
Expand All @@ -707,6 +739,8 @@ impl<T: Trait> Module<T> {
}

Self::deposit_event(RawEvent::Rollover(budget_remaining));

prior_approvals_len
}

/// Return the amount of money in the pot.
Expand Down
5 changes: 5 additions & 0 deletions frame/treasury/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ impl Contains<u64> for TenToFourteen {
})
}
}
impl ContainsCountUpperBound for TenToFourteen {
fn count_upper_bound() -> usize {
TEN_TO_FOURTEEN.with(|v| v.borrow().len())
}
}
parameter_types! {
pub const ProposalBond: Permill = Permill::from_percent(5);
pub const ProposalBondMinimum: u64 = 1;
Expand Down