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 3 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
7 changes: 3 additions & 4 deletions frame/treasury/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ fn create_approved_proposals<T: Trait>(n: u32) -> Result<(), &'static str> {
}

const MAX_BYTES: u32 = 16384;
const MAX_TIPPERS: u32 = 100;

benchmarks! {
_ { }
Expand Down Expand Up @@ -158,13 +157,13 @@ benchmarks! {

tip_new {
let r in 0 .. MAX_BYTES;
let t in 1 .. MAX_TIPPERS;
let t in 1 .. MAX_TIPPERS_COUNT;

let (caller, reason, beneficiary, value) = setup_tip::<T>(r, t)?;
}: _(RawOrigin::Signed(caller), reason, beneficiary, value)

tip {
let t in 1 .. MAX_TIPPERS;
let t in 1 .. MAX_TIPPERS_COUNT;
let (member, reason, beneficiary, value) = setup_tip::<T>(0, t)?;
let value = T::Currency::minimum_balance().saturating_mul(100.into());
Treasury::<T>::tip_new(
Expand All @@ -181,7 +180,7 @@ benchmarks! {
}: _(RawOrigin::Signed(caller), hash, value)

close_tip {
let t in 1 .. MAX_TIPPERS;
let t in 1 .. MAX_TIPPERS_COUNT;

// Make sure pot is funded
let pot_account = Treasury::<T>::account_id();
Expand Down
133 changes: 88 additions & 45 deletions frame/treasury/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,15 @@
use serde::{Serialize, Deserialize};
use sp_std::prelude::*;
use frame_support::{decl_module, decl_storage, decl_event, ensure, print, decl_error, Parameter};
use frame_support::dispatch::DispatchResultWithPostInfo;
use frame_support::traits::{
Currency, Get, Imbalance, OnUnbalanced, ExistenceRequirement::KeepAlive,
ReservableCurrency, WithdrawReason
};
use sp_runtime::{Permill, ModuleId, Percent, RuntimeDebug, traits::{
Zero, StaticLookup, AccountIdConversion, Saturating, Hash, BadOrigin
}};
use frame_support::weights::{Weight, MINIMUM_WEIGHT, SimpleDispatchInfo};
use frame_support::weights::Weight;
use frame_support::traits::{Contains, EnsureOrigin};
use codec::{Encode, Decode};
use frame_system::{self as system, ensure_signed, ensure_root};
Expand All @@ -113,6 +114,9 @@ type NegativeImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_sy
/// The treasury's module id, used for deriving its sovereign account ID.
const MODULE_ID: ModuleId = ModuleId(*b"py/trsry");

/// The maximum size of tippers, this is used to compute weight, the exceeding will be refund.
pub const MAX_TIPPERS_COUNT: u64 = 1000;
Comment thread
gui1117 marked this conversation as resolved.
Outdated

pub trait Trait: frame_system::Trait {
/// The staking balance.
type Currency: Currency<Self::AccountId> + ReservableCurrency<Self::AccountId>;
Expand All @@ -124,6 +128,8 @@ pub trait Trait: frame_system::Trait {
type RejectOrigin: EnsureOrigin<Self::Origin>;

/// Origin from which tippers must come.
/// The lenght must be less than [`MAX_TIPPERS_COUNT`](./constant.MAX_TIPPERS_COUNT.html).
/// Its cost it expected to be same as if it was stored sorted in a storage value.
type Tippers: Contains<Self::AccountId>;

/// The period for which a tip remains open after is has achieved threshold tippers.
Expand Down Expand Up @@ -323,11 +329,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 = SimpleDispatchInfo::FixedNormal(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 +356,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 = SimpleDispatchInfo::FixedOperational(100_000_000)]
#[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 +378,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 = SimpleDispatchInfo::FixedOperational(100_000_000)]
#[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 +406,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 = SimpleDispatchInfo::FixedNormal(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 +448,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 = SimpleDispatchInfo::FixedNormal(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,13 +480,20 @@ 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 less than `MAX_TIPPERS_COUNT`, it is charged as maximum and then refund.
/// 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 = SimpleDispatchInfo::FixedNormal(150_000_000)]
fn tip_new(origin, reason: Vec<u8>, who: T::AccountId, tip_value: BalanceOf<T>) {
#[weight = 107_700_000 + 4_000 * reason.len() as u64 + 481_000 * MAX_TIPPERS_COUNT
+ T::DbWeight::get().reads_writes(2, 2)]
fn tip_new(origin, reason: Vec<u8>, who: T::AccountId, tip_value: BalanceOf<T>)
-> DispatchResultWithPostInfo
{
// NOTE: we don't refund in case of error because checking cost too much.
let tipper = ensure_signed(origin)?;
ensure!(T::Tippers::contains(&tipper), BadOrigin);
let reason_hash = T::Hashing::hash(&reason[..]);
Expand All @@ -492,6 +505,8 @@ decl_module! {
let tips = vec![(tipper, tip_value)];
let tip = OpenTip { reason: reason_hash, who, finder: None, closes: None, tips };
Tips::<T>::insert(&hash, tip);

Ok(Some(MAX_TIPPERS_COUNT.saturating_sub(T::Tippers::count() as u64) * 481_000).into())
Comment thread
gui1117 marked this conversation as resolved.
Outdated
}

/// Declare a tip value for an already-open tip.
Expand All @@ -509,12 +524,19 @@ 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 less than `MAX_TIPPERS_COUNT`, it is charged as maximum and then refund.
/// 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 = SimpleDispatchInfo::FixedNormal(50_000_000)]
fn tip(origin, hash: T::Hash, tip_value: BalanceOf<T>) {
#[weight = 67_730_000 + 1_912_000 * MAX_TIPPERS_COUNT + T::DbWeight::get().reads_writes(2, 1)]
fn tip(origin, hash: T::Hash, tip_value: BalanceOf<T>) -> DispatchResultWithPostInfo {
// NOTE: we don't refund in case of error because checking cost too much.
let tipper = ensure_signed(origin)?;
ensure!(T::Tippers::contains(&tipper), BadOrigin);

Expand All @@ -523,6 +545,8 @@ decl_module! {
Self::deposit_event(RawEvent::TipClosing(hash.clone()));
}
Tips::<T>::insert(&hash, tip);

Ok(Some(MAX_TIPPERS_COUNT.saturating_sub(T::Tippers::count() as u64) * 1_912_000).into())
}

/// Close and payout a tip.
Expand All @@ -535,12 +559,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 less than `MAX_TIPPERS_COUNT`, it is charged as maximum and then refund.
/// The actual cost depends on the implementation of `T::Tippers`.
/// - DbReads: `Tips`, `Tippers`, `tip finder`
/// - DbWrites: `Reasons`, `Tips`, `Tippers`, `tip finder`
/// # </weight>
#[weight = SimpleDispatchInfo::FixedNormal(50_000_000)]
fn close_tip(origin, hash: T::Hash) {
#[weight = 211_000_000 + 1_094_000 * MAX_TIPPERS_COUNT + T::DbWeight::get().reads_writes(3, 3)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I liked your suggestion to include the raw data with the weight paragraph. I also think it was suggested we choose "round" numbers for weights, but maybe we should double check with others.

I would write a weight of:

 #[weight = 200_000_000 + 1_000_000 * MAX_TIPPERS_COUNT + T::DbWeight::get().reads_writes(3, 3)] 

If I was doing this.

@gui1117 gui1117 Apr 22, 2020

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.

it improves readibility but rounding 211 to 200 is 5% and 1_094 to 1_000 is 10%. I feel like its fine to just put the direct value with useless precision.

I rounded a bit.

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.

I kept 2 significative digits for each and rounded up to get them
https://en.wikipedia.org/wiki/Significant_figures

fn close_tip(origin, hash: T::Hash) -> DispatchResultWithPostInfo {
ensure_signed(origin)?;

let tip = Tips::<T>::get(hash).ok_or(Error::<T>::UnknownTip)?;
Expand All @@ -550,15 +577,27 @@ decl_module! {
Reasons::<T>::remove(&tip.reason);
Tips::<T>::remove(hash);
Self::payout_tip(hash, tip);

Ok(Some(MAX_TIPPERS_COUNT.saturating_sub(T::Tippers::count() as u64) * 1_094_000).into())
}

/// # <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
263_500_000 * approvals_len
+ T::DbWeight::get().reads_writes(2 + approvals_len * 3, 2 + approvals_len * 3)
} else {
0
}
}
}
}
Expand Down Expand Up @@ -650,14 +689,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 +721,7 @@ impl<T: Trait> Module<T> {
false
}
});
prior_approvals_len
});

if !missed_any {
Expand All @@ -707,6 +748,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