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 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
769c9bf
debug checkpoint.
kianenigma May 31, 2019
eb97440
new
kianenigma May 31, 2019
9a1b4f9
Worked.
kianenigma Jun 1, 2019
04a64ad
Worked and weight propagated to executive.
kianenigma Jun 3, 2019
0dad22b
Works with some tests.
kianenigma Jun 4, 2019
f546e10
Cleanup debug prints.
kianenigma Jun 5, 2019
89dd83b
Master.into()
kianenigma Jun 5, 2019
dea4232
More cleanup.
kianenigma Jun 5, 2019
2ba3087
Undo more logs.
kianenigma Jun 5, 2019
da9b861
Master.into()
kianenigma Jun 5, 2019
6f8f2e5
Undo a few more.
kianenigma Jun 5, 2019
64796e2
Fix build.
kianenigma Jun 5, 2019
07a72cb
Allow len to be used in weight calculation.
kianenigma Jun 5, 2019
f53ef4b
Remove noop function from dispath.
kianenigma Jun 5, 2019
620e64e
Cleanup.
kianenigma Jun 8, 2019
56495ab
Unify traits.
kianenigma Jun 11, 2019
ae124ea
Update docs and nits.
kianenigma Jun 11, 2019
389e225
line width
kianenigma Jun 11, 2019
66e957e
Update core/sr-primitives/src/weights.rs
kianenigma Jun 12, 2019
1658bc6
Update core/sr-primitives/src/weights.rs
kianenigma Jun 12, 2019
c05f0f9
Update core/sr-primitives/src/weights.rs
kianenigma Jun 12, 2019
18f8242
Update core/sr-primitives/src/weights.rs
kianenigma Jun 12, 2019
5f69fe4
Update srml/example/src/lib.rs
kianenigma Jun 12, 2019
152c1ab
Final cleanup.
kianenigma Jun 12, 2019
f14236b
Master.into()
kianenigma Jun 12, 2019
dcdad9d
Merge branch 'tx-weight-fee' of github.com:paritytech/substrate into …
kianenigma Jun 12, 2019
18cecd1
Fix build.
kianenigma Jun 12, 2019
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
3 changes: 3 additions & 0 deletions core/consensus/rhd/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@

#![cfg(feature="rhd")]
// FIXME #1020 doesn't compile
// NOTE: this is the legacy constant used for transaction size. No longer used except
// for the rhd code which is not updated. Placed here for compatibility.
const MAX_TRANSACTIONS_SIZE: u32 = 4 * 1024 * 1024;

use std::sync::Arc;
use std::sync::atomic::{AtomicUsize, Ordering};
Expand Down
3 changes: 0 additions & 3 deletions core/primitives/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ pub mod well_known_keys {
/// Current extrinsic index (u32) is stored under this key.
pub const EXTRINSIC_INDEX: &'static [u8] = b":extrinsic_index";

/// Sum of all lengths of executed extrinsics (u32).
pub const ALL_EXTRINSICS_LEN: &'static [u8] = b":all_extrinsics_len";

/// Changes trie configuration is stored under this key.
pub const CHANGES_TRIE_CONFIG: &'static [u8] = b":changes_trie";

Expand Down
13 changes: 11 additions & 2 deletions core/sr-primitives/src/generic/checked_extrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//! stage.

use crate::traits::{self, Member, SimpleArithmetic, MaybeDisplay};
use crate::weights::{WeighableCall, Weight};

/// Definition of something that the external world might want to say; its
/// existence implies that it has been checked and is good, particularly with
Expand All @@ -32,8 +33,7 @@ pub struct CheckedExtrinsic<AccountId, Index, Call> {
pub function: Call,
}

impl<AccountId, Index, Call> traits::Applyable
for CheckedExtrinsic<AccountId, Index, Call>
impl<AccountId, Index, Call> traits::Applyable for CheckedExtrinsic<AccountId, Index, Call>
where
AccountId: Member + MaybeDisplay,
Index: Member + MaybeDisplay + SimpleArithmetic,
Expand All @@ -55,3 +55,12 @@ where
(self.function, self.signed.map(|x| x.0))
}
}

impl<AccountId, Index, Call> WeighableCall for CheckedExtrinsic<AccountId, Index, Call>
where
Call: WeighableCall,
{
fn weight(&self, len: usize) -> Weight {
self.function.weight(len)
}
}
1 change: 1 addition & 0 deletions core/sr-primitives/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use codec::{Encode, Decode};
#[cfg(feature = "std")]
pub mod testing;

pub mod weights;
pub mod traits;
use traits::{SaturatedConversion, UniqueSaturatedInto};

Expand Down
6 changes: 6 additions & 0 deletions core/sr-primitives/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use serde::{Serialize, Serializer, Deserialize, de::Error as DeError, Deserializ
use std::{fmt::Debug, ops::Deref, fmt};
use crate::codec::{Codec, Encode, Decode};
use crate::traits::{self, Checkable, Applyable, BlakeTwo256, Convert};
use crate::weights::{WeighableCall, Weight};
use crate::generic::DigestItem as GenDigestItem;
pub use substrate_primitives::H256;
use substrate_primitives::U256;
Expand Down Expand Up @@ -239,3 +240,8 @@ impl<Call> Applyable for TestXt<Call> where
(self.2, self.0)
}
}
impl<Call> WeighableCall for TestXt<Call> {
fn weight(&self, len: usize) -> Weight {
len as Weight
}
}
85 changes: 85 additions & 0 deletions core/sr-primitives/src/weights.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2019 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 <http://www.gnu.org/licenses/>.

//! Primitives for transaction weighting.
//!
//! Each dispatch function withing `decl_module!` can now have an optional
//! `#[weight = $x]` attribute. $x can be any object that implements the
//! [`WeighableTransaction`] trait. By default, All transactions are annotated by
//! `#[weight = TransactionWeight::default()]`.
//!
//! Note that the decl_module macro _cannot_ enforce this and will simply fail
//! if an invalid struct is passed in.
//!
//! Note that [`WeighableCall`] and [`WeighableTransaction`] are more or less similar.
//! The distinction is because one serves to pass the weight from the the
//! dispatchable function's attribute to the call enum ([`WeighableTransaction`]) and the
//! other to pass the final weight from call enum to the executive module
//! ([`WeighableCall`]).
/// The final type that each `#[weight = $x:expr]`'s
/// expression must evaluate to.
pub type Weight = u32;

/// A `Call` enum that can be weighted using the custom weight attribute of the
/// its dispatchable functions. Is implemented by default in the `decl_module!`.
pub trait WeighableCall {
/// Return the weight of this call.
fn weight(&self, len: usize) -> Weight;
}

/// a _dispatchable_ function (anything inside `decl_module! {}`) that can be weighted.
/// A type implementing this trait can _optionally_ be passed to the as
/// `#[weight = X]`. Otherwise, default implementation will be used.
pub trait WeighableTransaction {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel this trait is very useful. The WeighableCall interfaces is limitting this stuff to linear cost of base and bytes , I think that it might be worth allowing users to specify different cost calculations that are based on the call and it's encoded len, which is not possible currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it's unnecessary. WeighableCall and WeighableTransaction look almost identical - they just have different method names and in transaciton self is consumed (for no apparent reason).

I'd unify this into Weighable and implement for multiple things if needed.

/// Consume self and return the final weight of the call given the length
/// of the extrinsic.
fn calculate_weight(self, len: usize) -> Weight;
}

/// Default weight wrapper.
/// This is tailored for the Polkadot use case. Users may replace it with anything.
pub enum TransactionWeight {
/// basic weight (base, byte).
/// The values contained are the base weight and byte weight respectively.
Basic(Weight, Weight),
/// Maximum fee. This implies that this transaction _might_ get included but
/// no more transaction can be added. This can be done by setting the
/// implementation to _maximum block weight_.
Max,
/// Free. The transaction does not increase the total weight
/// (i.e. is not included in weight calculation).
Free,
}

impl WeighableTransaction for TransactionWeight {
fn calculate_weight(self, len: usize) -> Weight {
match self {
TransactionWeight::Basic(base, byte) => base + byte * len as Weight,
TransactionWeight::Max => 3 * 1024 * 1024,
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realised that we could calculate max given current block weight (we would have to pass it to the function along with len) - then you could just return whatever is left in a block.

I don't like it that much though, due to additional argument that has to be passed so if we have a better solution for longer term or we are satisfied enough with this, then let's just leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you propose will actually work (haven't tried but shouldn't have a problem).

The potential long term solution is the other approach I explained here. I think in that design the weight calculation is internal to the Module<T> and can simply query system. I am still not fully convinced of how that should work (+ if it is absolutely needed) hence rather get this stable and clean first to be able to proceed to fee + tip steps and then refactor if needed (hopefully a decision I won't regret ♻️)

TransactionWeight::Free => 0,
}
}
}

impl Default for TransactionWeight {
fn default() -> Self {
// This implies that the weight is currently equal to tx-size, nothing more
// for all substrate transactions that do NOT explicitly annotate weight.
// TODO #2431 needs to be updated with proper max values.
TransactionWeight::Basic(0, 1)
}
}
1 change: 0 additions & 1 deletion core/state-machine/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,6 @@ mod tests {
).execute_using_consensus_failure_handler::<_, NeverNativeValue, fn() -> _>(
ExecutionManager::Both(|we, _ne| {
consensus_failed = true;
println!("HELLO!");
we
}),
true,
Expand Down
2 changes: 1 addition & 1 deletion node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
impl_name: create_runtime_str!("substrate-node"),
authoring_version: 10,
spec_version: 92,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should bump the spec.

impl_version: 92,
impl_version: 93,
apis: RUNTIME_API_VERSIONS,
};

Expand Down
2 changes: 1 addition & 1 deletion srml/democracy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.

//! Democratic system: Handles administration of general stakeholder voting.

#![recursion_limit="128"]
#![cfg_attr(not(feature = "std"), no_std)]

use rstd::prelude::*;
Expand Down
2 changes: 1 addition & 1 deletion srml/example/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ parity-codec = { version = "3.3", default-features = false }
srml-support = { path = "../support", default-features = false }
system = { package = "srml-system", path = "../system", default-features = false }
balances = { package = "srml-balances", path = "../balances", default-features = false }
sr-primitives = { path = "../../core/sr-primitives", default-features = false }

[dev-dependencies]
sr-io = { path = "../../core/sr-io" }
substrate-primitives = { path = "../../core/primitives" }
sr-primitives = { path = "../../core/sr-primitives" }

[features]
default = ["std"]
Expand Down
20 changes: 17 additions & 3 deletions srml/example/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

//! # Example Module
//!
//! <!-- Original author of paragraph: @gavofyork -->
//! <!-- Original author of paragraph: @gavofyork -->
//! The Example: A simple example of a runtime module demonstrating
//! concepts, APIs and structures common to most runtime modules.
//!
Expand Down Expand Up @@ -64,7 +64,7 @@
//!
//! \## Overview
//!
//! <!-- Original author of paragraph: Various. See https://github.com/paritytech/substrate-developer-hub/issues/44 -->
//! <!-- Original author of paragraph: Various. See https://github.com/paritytech/substrate-developer-hub/issues/44 -->
//! // Short description of module purpose.
//! // Links to Traits that should be implemented.
//! // What this module is for.
Expand Down Expand Up @@ -205,7 +205,7 @@
//!
//! \```rust
//! use <INSERT_CUSTOM_MODULE_NAME>;
//!
//!
//! pub trait Trait: <INSERT_CUSTOM_MODULE_NAME>::Trait { }
//! \```
//!
Expand Down Expand Up @@ -251,6 +251,7 @@

use srml_support::{StorageValue, dispatch::Result, decl_module, decl_storage, decl_event};
use system::ensure_signed;
use sr_primitives::weights::TransactionWeight;

/// Our module's configuration trait. All our types and consts go in here. If the
/// module is dependent on specific other modules, then their configuration traits
Expand Down Expand Up @@ -388,6 +389,19 @@ decl_module! {
// no progress.
//
// If you don't respect these rules, it is likely that your chain will be attackable.
//
// Each transaction can optionally indicate a weight. The weight is passed in as a
// custom attribute and the value can be anything that implements the `WeighableTransaction`
// trait. Most often using substrate's default `TransactionWeight` is enough for you. A basic
// weight is a tuple of `(base_weight, byte_weight)`. Upon including each transaction in a block,
// the final weight is calculated as `base_weight + byte_weight * tx_size`. If this value,
// added to the weight of all included transactions, exceeds `MAX_TRANSACTION_WEIGHT`,
// the transaction is not included. If no weight attribute is provided, the `::default()`
// implementation of `TransactionWeight()` is used.
//
// The example below showcases a transaction which is relatively costly, but less dependent on
// the input, hence byte_weight is configured smaller.
#[weight = TransactionWeight::Basic(10_000, 100)]
fn accumulate_dummy(origin, increase_by: T::Balance) -> Result {
// This is a public call, so we ensure that the origin is some signed account.
let _sender = ensure_signed(origin)?;
Expand Down
Loading