Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Operational Transaction. #3106

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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: 7 additions & 0 deletions core/sr-primitives/src/generic/checked_extrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

use crate::traits::{self, Member, SimpleArithmetic, MaybeDisplay};
use crate::weights::{Weighable, Weight};
use crate::transaction_validity::TransactionPriority;

/// 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 Down Expand Up @@ -63,4 +64,10 @@ where
fn weight(&self, len: usize) -> Weight {
self.function.weight(len)
}
fn priority(&self, len: usize) -> TransactionPriority {
self.function.priority(len)
}
fn is_block_full(&self, block_weight: Weight, len: usize) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay this is not a great name since it is misleading. Should be something along the lines of xt.will_cause_full_block() -> bool

self.function.is_block_full(block_weight, len)
}
}
11 changes: 9 additions & 2 deletions core/sr-primitives/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use std::{fmt::Debug, ops::Deref, fmt};
use crate::codec::{Codec, Encode, Decode};
use crate::traits::{self, Checkable, Applyable, BlakeTwo256, OpaqueKeys, TypedKey};
use crate::{generic, KeyTypeId};
use crate::weights::{Weighable, Weight};
use crate::weights::{Weighable, Weight, MAX_TRANSACTIONS_WEIGHT};
use crate::transaction_validity::TransactionPriority;
pub use substrate_primitives::H256;
use substrate_primitives::U256;
use substrate_primitives::ed25519::{Public as AuthorityId};
Expand Down Expand Up @@ -237,7 +238,13 @@ impl<Call> Applyable for TestXt<Call> where
}
impl<Call> Weighable for TestXt<Call> {
fn weight(&self, len: usize) -> Weight {
// for testing: weight == size.
len as Weight
}
fn priority(&self, len: usize) -> TransactionPriority {
len as TransactionPriority
}
fn is_block_full(&self, current_weight: Weight, len: usize)
-> bool {
current_weight + self.weight(len) > MAX_TRANSACTIONS_WEIGHT
}
}
43 changes: 34 additions & 9 deletions core/sr-primitives/src/weights.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@
//! Note that the decl_module macro _cannot_ enforce this and will simply fail
//! if an invalid struct is passed in.

use crate::transaction_validity::TransactionPriority;

/// The final type that each `#[weight = $x:expr]`'s
/// expression must evaluate to.
pub type Weight = u32;

/// The maximum possible weight of a block.
pub const MAX_TRANSACTIONS_WEIGHT: u32 = 4 * 1024 * 1024;

/// A `Call` enum (aka transaction) that can be weighted using the custom weight attribute of
/// its dispatchable functions. Is implemented by default in the `decl_module!`.
///
Expand All @@ -37,6 +42,13 @@ pub trait Weighable {
/// Return the weight of this call.
/// The `len` argument is the encoded length of the transaction/call.
fn weight(&self, len: usize) -> Weight;

/// Return the priority of this transaction.
fn priority(&self, len: usize) -> TransactionPriority;

/// Determine, given the current transaction weight and sum of weights in the current block, if
/// the block is now full or not.
fn is_block_full(&self, block_weight: Weight, len: usize) -> bool;
}

/// Default type used as the weight representative in a `#[weight = x]` attribute.
Expand All @@ -46,22 +58,35 @@ pub trait Weighable {
pub enum TransactionWeight {
/// Basic weight (base, byte).
/// The values contained are the base weight and byte weight respectively.
///
/// The priority of this transaction will be proportional to its computed weight.
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,
/// Operational transaction. These are typically root transactions for operational updates,
/// runtime code changes or consensus reports through inherents. These transactions are still
/// subject to the same (base, byte) fee but will have maximum priority and will not affect
/// block fullness at all.
Operational(Weight, Weight),
}

impl Weighable for TransactionWeight {
fn weight(&self, len: usize) -> Weight {
match self {
TransactionWeight::Basic(base, byte) => base + byte * len as Weight,
TransactionWeight::Max => 3 * 1024 * 1024,
TransactionWeight::Free => 0,
TransactionWeight::Operational(_, _) => 0,
}
}

fn priority(&self, len: usize) -> TransactionPriority {
match self {
TransactionWeight::Basic(_, _) => self.weight(len) as TransactionPriority,
TransactionWeight::Operational(_, _) => TransactionPriority::max_value(),
}
}

fn is_block_full(&self, block_weight: Weight, len: usize) -> bool {
match self {
TransactionWeight::Basic(_, _) => self.weight(len) + block_weight > MAX_TRANSACTIONS_WEIGHT,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TransactionWeight::Basic(_, _) => self.weight(len) + block_weight > MAX_TRANSACTIONS_WEIGHT,
TransactionWeight::Basic(_, _) => self.weight(len).checked_add(block_weight).map(|weight| weight > MAX_TRANSACTIONS_WEIGHT).unwrap_or(true),

TransactionWeight::Operational(_, _) => false
}
}
}
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 @@ -70,7 +70,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
// implementation changes and behavior does not, then leave spec_version as
// is and increment impl_version.
spec_version: 109,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
spec_version: 109,
spec_version: 111,

impl_version: 110,
impl_version: 111,
apis: RUNTIME_API_VERSIONS,
};

Expand Down
14 changes: 7 additions & 7 deletions srml/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,10 @@ use srml_support::{Dispatchable, traits::MakePayment};
use parity_codec::{Codec, Encode};
use system::{extrinsics_root, DigestOf};
use primitives::{ApplyOutcome, ApplyError};
use primitives::transaction_validity::{TransactionValidity, TransactionPriority, TransactionLongevity};
use primitives::transaction_validity::{TransactionValidity, TransactionLongevity};
use primitives::weights::Weighable;

mod internal {
pub const MAX_TRANSACTIONS_WEIGHT: u32 = 4 * 1024 * 1024;

pub enum ApplyError {
BadSignature(&'static str),
Stale,
Expand Down Expand Up @@ -265,8 +263,8 @@ where
let xt = uxt.check(&Default::default()).map_err(internal::ApplyError::BadSignature)?;

// Check the weight of the block if that extrinsic is applied.
let weight = xt.weight(encoded_len);
if <system::Module<System>>::all_extrinsics_weight() + weight > internal::MAX_TRANSACTIONS_WEIGHT {
let block_weight = <system::Module<System>>::all_extrinsics_weight();
if <CheckedOf<Block::Extrinsic, Context> as Weighable>::is_block_full(&xt, block_weight, encoded_len) {
return Err(internal::ApplyError::FullBlock);
}

Expand All @@ -277,6 +275,7 @@ where
if index < &expected_index { internal::ApplyError::Stale } else { internal::ApplyError::Future }
) }
// pay any fees
// TODO TODO: use weight to deduct the fee once #2854 is merged.
Payment::make_payment(sender, encoded_len).map_err(|_| internal::ApplyError::CantPay)?;

// AUDIT: Under no circumstances may this function panic from here onwards.
Expand Down Expand Up @@ -370,7 +369,7 @@ where
};

TransactionValidity::Valid {
priority: encoded_len as TransactionPriority,
priority: xt.priority(encoded_len),
requires,
provides,
longevity: TransactionLongevity::max_value(),
Expand All @@ -397,6 +396,7 @@ mod tests {
use substrate_primitives::{H256, Blake2Hasher};
use primitives::traits::{Header as HeaderT, BlakeTwo256, IdentityLookup};
use primitives::testing::{Digest, Header, Block};
use primitives::weights::MAX_TRANSACTIONS_WEIGHT;
use srml_support::{impl_outer_event, impl_outer_origin, parameter_types};
use srml_support::traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReasons, WithdrawReason};
use system;
Expand Down Expand Up @@ -586,7 +586,7 @@ mod tests {
let xt = primitives::testing::TestXt(Some(1), 0, Call::transfer(33, 69));
let xt2 = primitives::testing::TestXt(Some(1), 1, Call::transfer(33, 69));
let encoded = xt2.encode();
let len = if should_fail { (internal::MAX_TRANSACTIONS_WEIGHT - 1) as usize } else { encoded.len() };
let len = if should_fail { (MAX_TRANSACTIONS_WEIGHT - 1) as usize } else { encoded.len() };
with_externalities(&mut t, || {
Executive::initialize_block(&Header::new(
1,
Expand Down
44 changes: 39 additions & 5 deletions srml/support/src/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub use srml_metadata::{
ModuleConstantMetadata, DefaultByte, DefaultByteGetter,
};
pub use sr_primitives::weights::{TransactionWeight, Weighable, Weight};
pub use sr_primitives::transaction_validity::TransactionPriority;

/// A type that cannot be instantiated.
pub enum Never {}
Expand Down Expand Up @@ -1126,6 +1127,22 @@ macro_rules! decl_module {
$call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") },
}
}
fn priority(&self, _len: usize) -> $crate::dispatch::TransactionPriority {
match self {
$( $call_type::$fn_name(..) => $crate::dispatch::Weighable::priority(&$weight, _len), )*
$call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") },
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ensured at compile-time. For example, __PhantomItem could contain Infallible, and then the unreachable! could be replaced by a pattern-match against it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkchr this pattern can be replaced probably everywhere in storage macros? I borrowed the unreachable!() from other places in the code.

}
}
fn is_block_full(&self, _block_weight: $crate::dispatch::Weight, _len: usize) -> bool {
match self {
$( $call_type::$fn_name(..) => $crate::dispatch::Weighable::is_block_full(
&$weight,
_block_weight,
_len,
), )*
$call_type::__PhantomItem(_, _) => { unreachable!("__PhantomItem should never be used.") },
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

}
}
}

// manual implementation of clone/eq/partialeq because using derive erroneously requires
Expand Down Expand Up @@ -1275,6 +1292,17 @@ macro_rules! impl_outer_dispatch {
$( $call_type::$camelcase(call) => call.weight(len), )*
}
}
fn priority(&self, len: usize) -> $crate::dispatch::TransactionPriority {
match self {
$( $call_type::$camelcase(call) => call.priority(len), )*
}
}
fn is_block_full(&self, block_weight: $crate::dispatch::Weight, len: usize) -> bool {
match self {

$( $call_type::$camelcase(call) => call.is_block_full(block_weight, len), )*
}
}
}
impl $crate::dispatch::Dispatchable for $call_type {
type Origin = $origin;
Expand Down Expand Up @@ -1613,8 +1641,8 @@ mod tests {
fn on_finalize(n: T::BlockNumber) { if n.into() == 42 { panic!("on_finalize") } }
fn offchain_worker() {}

#[weight = TransactionWeight::Max]
fn weighted(_origin) { unreachable!() }
#[weight = TransactionWeight::Operational(0, 1)]
fn security_tx(_origin) { unreachable!() }
}
}

Expand Down Expand Up @@ -1680,7 +1708,8 @@ mod tests {
documentation: DecodeDifferent::Encode(&[]),
},
FunctionMetadata {
name: DecodeDifferent::Encode("weighted"),

name: DecodeDifferent::Encode("security_tx"),
arguments: DecodeDifferent::Encode(&[]),
documentation: DecodeDifferent::Encode(&[]),
},
Expand Down Expand Up @@ -1754,10 +1783,15 @@ mod tests {

#[test]
fn weight_should_attach_to_call_enum() {
// max weight. not dependent on input.
assert_eq!(Call::<TraitImpl>::weighted().weight(100), 3 * 1024 * 1024);
// Operational tx.
assert_eq!(Call::<TraitImpl>::security_tx().weight(100), 0);
assert_eq!(Call::<TraitImpl>::security_tx().priority(100), u64::max_value());
// one simply does not return false of the transaction type is operational!.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// one simply does not return false of the transaction type is operational!.
// Operational transactions are never denied no matter how full
// the block 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.

fair enough. I am personally pretty much okay with having some humor/informal statements as long as it is in test code :p

assert_eq!(Call::<TraitImpl>::security_tx().is_block_full(u32::max_value(), usize::max_value()), false);

// default weight.
assert_eq!(Call::<TraitImpl>::aux_0().weight(5), 5 /*tx-len*/);

// custom basic
assert_eq!(Call::<TraitImpl>::aux_3().weight(5), 10 + 100 * 5 );
}
Expand Down