From 1e0dc2613b8174b64b9f004b3d85cc823d2abc7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 14 Sep 2021 16:16:05 +0200 Subject: [PATCH 01/14] Add transaction validity docs. --- frame/system/src/extensions/check_genesis.rs | 5 +++++ frame/system/src/extensions/check_mortality.rs | 4 ++++ frame/system/src/extensions/check_nonce.rs | 7 +++++-- frame/system/src/extensions/check_spec_version.rs | 5 +++++ frame/system/src/extensions/check_tx_version.rs | 5 +++++ frame/system/src/extensions/check_weight.rs | 10 ++++++++++ frame/transaction-payment/src/lib.rs | 6 ++++++ 7 files changed, 40 insertions(+), 2 deletions(-) diff --git a/frame/system/src/extensions/check_genesis.rs b/frame/system/src/extensions/check_genesis.rs index 6f409d5d3d4ad..9c5c890ee6098 100644 --- a/frame/system/src/extensions/check_genesis.rs +++ b/frame/system/src/extensions/check_genesis.rs @@ -24,6 +24,11 @@ use sp_runtime::{ }; /// Genesis hash check to provide replay protection between different networks. +/// +/// # Transaction Validity +/// +/// Note that while a transaction with invalid `genesis_hash` will fail to be decoded, +/// the extension does not affect any other fields of `TransactionValidity` directly. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckGenesis(sp_std::marker::PhantomData); diff --git a/frame/system/src/extensions/check_mortality.rs b/frame/system/src/extensions/check_mortality.rs index 69cca765efea9..941f28dc6fc63 100644 --- a/frame/system/src/extensions/check_mortality.rs +++ b/frame/system/src/extensions/check_mortality.rs @@ -27,6 +27,10 @@ use sp_runtime::{ }; /// Check for transaction mortality. +/// +/// # Transaction Validity +/// +/// The extension affects `longevity` of the transaction according to the [`Era`] definition. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckMortality(Era, sp_std::marker::PhantomData); diff --git a/frame/system/src/extensions/check_nonce.rs b/frame/system/src/extensions/check_nonce.rs index 74be83398421e..3c6f9a1b4dbd1 100644 --- a/frame/system/src/extensions/check_nonce.rs +++ b/frame/system/src/extensions/check_nonce.rs @@ -30,8 +30,11 @@ use sp_std::vec; /// Nonce check and increment to give replay protection for transactions. /// -/// Note that this does not set any priority by default. Make sure that AT LEAST one of the signed -/// extension sets some kind of priority upon validating transactions. +/// # Transaction Validity +/// +/// This extension affects `requires` and `provides` tags of validity, but DOES NOT +/// set the `priority` field. Make sure that AT LEAST one of the signed extension sets +/// some kind of priority upon validating transactions. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckNonce(#[codec(compact)] pub T::Index); diff --git a/frame/system/src/extensions/check_spec_version.rs b/frame/system/src/extensions/check_spec_version.rs index 0217aefae6b9d..688abe99763a2 100644 --- a/frame/system/src/extensions/check_spec_version.rs +++ b/frame/system/src/extensions/check_spec_version.rs @@ -21,6 +21,11 @@ use scale_info::TypeInfo; use sp_runtime::{traits::SignedExtension, transaction_validity::TransactionValidityError}; /// Ensure the runtime version registered in the transaction is the same as at present. +/// +/// # Transaction Validity +/// +/// The transaction with incorrect `spec_version` are considered invalid. The validity +/// is not affected in any other way. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckSpecVersion(sp_std::marker::PhantomData); diff --git a/frame/system/src/extensions/check_tx_version.rs b/frame/system/src/extensions/check_tx_version.rs index 9418d3ff5d937..f6bb53e1cba34 100644 --- a/frame/system/src/extensions/check_tx_version.rs +++ b/frame/system/src/extensions/check_tx_version.rs @@ -21,6 +21,11 @@ use scale_info::TypeInfo; use sp_runtime::{traits::SignedExtension, transaction_validity::TransactionValidityError}; /// Ensure the transaction version registered in the transaction is the same as at present. +/// +/// # Transaction Validity +/// +/// The transaction with incorrect `transaction_version` are considered invalid. The validity +/// is not affected in any other way. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckTxVersion(sp_std::marker::PhantomData); diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 92dc7382fa2d5..b93d5b3cc12dd 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -32,6 +32,16 @@ use sp_runtime::{ }; /// Block resource (weight) limit check. +/// +/// # Transaction Validity +/// +/// The extension affects `priority` field of `TransationValidity`. The priority depends +/// on the `DispatchClass` and declared `weight`. +/// +/// Note that usually it's desireable to include some fee payment mechanism and use that +/// as a primary source of `priority`. Take a look at +/// [`frame_support::signed_extensions::AdjustPriority`] utility to adjust priority coming +/// from multiple extensions. #[derive(Encode, Decode, Clone, Eq, PartialEq, Default, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckWeight(sp_std::marker::PhantomData); diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index e3a3bccc3d39a..bee53d1f1d885 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -523,6 +523,12 @@ where /// Require the transactor pay for themselves and maybe include a tip to gain additional priority /// in the queue. +/// +/// # Transaction Validity +/// +/// This extension sets the `priority` field of `TransactionValidity` depending on the amount +/// of fee being paid (including the tip) and byte size. Transactions with high per-weight-unit +/// fee or per-byte fee (depending which is greater) will have higher priority. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct ChargeTransactionPayment(#[codec(compact)] BalanceOf); From fe93e9e7a528ab99db766d1cfa5b741b78398b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 21 Sep 2021 15:44:05 +0200 Subject: [PATCH 02/14] Re-work priority calculation. --- Cargo.lock | 8 +- bin/node-template/runtime/src/lib.rs | 2 + bin/node/runtime/src/lib.rs | 2 + frame/executive/src/lib.rs | 4 +- frame/support/src/weights.rs | 24 ---- frame/system/src/extensions/check_weight.rs | 62 +-------- frame/transaction-payment/src/lib.rs | 139 +++++++++++++++++--- 7 files changed, 134 insertions(+), 107 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 839c68b71fbf1..8bbfc9ff4438a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2412,9 +2412,9 @@ dependencies = [ [[package]] name = "git2" -version = "0.13.21" +version = "0.13.22" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "659cd14835e75b64d9dba5b660463506763cf0aa6cb640aeeb0e98d841093490" +checksum = "9c1cbbfc9a1996c6af82c2b4caf828d2c653af4fcdbb0e5674cc966eee5a4197" dependencies = [ "bitflags", "libc", @@ -3233,9 +3233,9 @@ checksum = "789da6d93f1b866ffe175afc5322a4d76c038605a1c3319bb57b06967ca98a36" [[package]] name = "libgit2-sys" -version = "0.12.22+1.1.0" +version = "0.12.23+1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89c53ac117c44f7042ad8d8f5681378dfbc6010e49ec2c0d1f11dfedc7a4a1c3" +checksum = "29730a445bae719db3107078b46808cc45a5b7a6bae3f31272923af969453356" dependencies = [ "cc", "libc", diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index eecc93e166666..3ffb3c0098d4d 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -257,11 +257,13 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 1; + pub WeightTipRatio: Balance = BlockWeights::get().max_block as Balance / ExistentialDeposit::get(); } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 28f8e5dc3fd6a..cd23cca6ae300 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -412,6 +412,7 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 10 * MILLICENTS; + pub const WeightTipRatio: Balance = MAXIMUM_BLOCK_WEIGHT as Balance / 1 * DOLLARS; pub const TargetBlockFullness: Perquintill = Perquintill::from_percent(25); pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(1, 100_000); pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000_000u128); @@ -420,6 +421,7 @@ parameter_types! { impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = TargetedFeeAdjustment; diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 9a0fce4d6b5b4..451b4d1942926 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -1054,8 +1054,6 @@ mod tests { let invalid = TestXt::new(Call::Custom(custom::Call::unallowed_unsigned {}), None); let mut t = new_test_ext(1); - let mut default_with_prio_3 = ValidTransaction::default(); - default_with_prio_3.priority = 3; t.execute_with(|| { assert_eq!( Executive::validate_transaction( @@ -1063,7 +1061,7 @@ mod tests { valid.clone(), Default::default(), ), - Ok(default_with_prio_3), + Ok(ValidTransaction::default()), ); assert_eq!( Executive::validate_transaction( diff --git a/frame/support/src/weights.rs b/frame/support/src/weights.rs index 115470a9bf034..ec5f37823ad47 100644 --- a/frame/support/src/weights.rs +++ b/frame/support/src/weights.rs @@ -287,30 +287,6 @@ impl<'a> OneOrMany for &'a [DispatchClass] { } } -/// Primitives related to priority management of Frame. -pub mod priority { - /// The starting point of all Operational transactions. 3/4 of u64::MAX. - pub const LIMIT: u64 = 13_835_058_055_282_163_711_u64; - - /// Wrapper for priority of different dispatch classes. - /// - /// This only makes sure that any value created for the operational dispatch class is - /// incremented by [`LIMIT`]. - pub enum FrameTransactionPriority { - Normal(u64), - Operational(u64), - } - - impl From for u64 { - fn from(priority: FrameTransactionPriority) -> Self { - match priority { - FrameTransactionPriority::Normal(inner) => inner, - FrameTransactionPriority::Operational(inner) => inner.saturating_add(LIMIT), - } - } - } -} - /// A bundle of static information collected from the `#[weight = $x]` attributes. #[derive(Clone, Copy, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode, TypeInfo)] pub struct DispatchInfo { diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index b93d5b3cc12dd..152d9f928883d 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -19,14 +19,13 @@ use crate::{limits::BlockWeights, Config, Pallet}; use codec::{Decode, Encode}; use frame_support::{ traits::Get, - weights::{priority::FrameTransactionPriority, DispatchClass, DispatchInfo, PostDispatchInfo}, + weights::{DispatchClass, DispatchInfo, PostDispatchInfo}, }; use scale_info::TypeInfo; use sp_runtime::{ traits::{DispatchInfoOf, Dispatchable, PostDispatchInfoOf, SignedExtension}, transaction_validity::{ - InvalidTransaction, TransactionPriority, TransactionValidity, TransactionValidityError, - ValidTransaction, + InvalidTransaction, TransactionValidity, TransactionValidityError, }, DispatchResult, }; @@ -35,13 +34,8 @@ use sp_runtime::{ /// /// # Transaction Validity /// -/// The extension affects `priority` field of `TransationValidity`. The priority depends -/// on the `DispatchClass` and declared `weight`. -/// -/// Note that usually it's desireable to include some fee payment mechanism and use that -/// as a primary source of `priority`. Take a look at -/// [`frame_support::signed_extensions::AdjustPriority`] utility to adjust priority coming -/// from multiple extensions. +/// This extension does not influence any fields of `TransactionValidity` in case the +/// transaction is valid. #[derive(Encode, Decode, Clone, Eq, PartialEq, Default, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckWeight(sp_std::marker::PhantomData); @@ -91,23 +85,6 @@ where } } - /// Get the priority of an extrinsic denoted by `info`. - /// - /// Operational transaction will be given a fixed initial amount to be fairly distinguished from - /// the normal ones. - fn get_priority(info: &DispatchInfoOf) -> TransactionPriority { - match info.class { - // Normal transaction. - DispatchClass::Normal => FrameTransactionPriority::Normal(info.weight.into()).into(), - // Don't use up the whole priority space, to allow things like `tip` to be taken into - // account as well. - DispatchClass::Operational => - FrameTransactionPriority::Operational(info.weight.into()).into(), - // Mandatory extrinsics are only for inherents; never transactions. - DispatchClass::Mandatory => TransactionPriority::min_value(), - } - } - /// Creates new `SignedExtension` to check weight of the extrinsic. pub fn new() -> Self { Self(Default::default()) @@ -140,7 +117,7 @@ where // consumption from causing false negatives. Self::check_extrinsic_weight(info)?; - Ok(ValidTransaction { priority: Self::get_priority(info), ..Default::default() }) + Ok(Default::default()) } } @@ -380,10 +357,7 @@ mod tests { assert_eq!( CheckWeight::::do_validate(&okay, len), - Ok(ValidTransaction { - priority: CheckWeight::::get_priority(&okay), - ..Default::default() - }) + Ok(Default::default()) ); assert_err!( CheckWeight::::do_validate(&max, len), @@ -516,30 +490,6 @@ mod tests { }) } - #[test] - fn signed_ext_check_weight_works() { - new_test_ext().execute_with(|| { - let normal = - DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; - let op = DispatchInfo { - weight: 100, - class: DispatchClass::Operational, - pays_fee: Pays::Yes, - }; - let len = 0_usize; - - let priority = CheckWeight::(PhantomData) - .validate(&1, CALL, &normal, len) - .unwrap() - .priority; - assert_eq!(priority, 100); - - let priority = - CheckWeight::(PhantomData).validate(&1, CALL, &op, len).unwrap().priority; - assert_eq!(priority, frame_support::weights::priority::LIMIT + 100); - }) - } - #[test] fn signed_ext_check_weight_block_size_works() { new_test_ext().execute_with(|| { diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index bee53d1f1d885..29b04eb7aec12 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -264,6 +264,17 @@ pub mod pallet { #[pallet::constant] type TransactionByteFee: Get>; + /// The scaling factor for `priority` calculations. + /// + /// The priority is calculated by (integer) dividing the `tip` being payed and the amount of + /// `weight` the transaction consumes. Depending on the real-world values of both + /// `Balance` and `Weight` this division might end up in undesirable range. + /// To accomodated that, this additional scaling factor will be included in computations, + /// i.e. `tip * WeightTipRatio / weight`. + /// Recommended value for this constant is `MAXIMUM_BLOCK_WEIGHT / AVERAGE_TIP_AMOUNT`. + #[pallet::constant] + type WeightTipRatio: Get>; + /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: WeightToFeePolynomial>; @@ -527,8 +538,10 @@ where /// # Transaction Validity /// /// This extension sets the `priority` field of `TransactionValidity` depending on the amount -/// of fee being paid (including the tip) and byte size. Transactions with high per-weight-unit -/// fee or per-byte fee (depending which is greater) will have higher priority. +/// of tip being paid per weight unit. +/// +/// Operational transactions will receive an additional priority bump, so that they are normally +/// considered before regular transactions. #[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct ChargeTransactionPayment(#[codec(compact)] BalanceOf); @@ -570,27 +583,57 @@ where .map(|i| (fee, i)) } - /// Get an appropriate priority for a transaction with the given length and info. + /// Get an appropriate priority for a transaction with the given `DispatchInfo` and fee to be + /// paid (with `tip`). /// - /// This will try and optimise the `fee/weight` `fee/length`, whichever is consuming more of the - /// maximum corresponding limit. + /// Values returned by the function depend on the following: + /// 1. The average per-weight-unit tip to be payed. + /// 2. The dispatch class of the transaction. /// - /// For example, if a transaction consumed 1/4th of the block length and half of the weight, its - /// final priority is `fee * min(2, 4) = fee * 2`. If it consumed `1/4th` of the block length - /// and the entire block weight `(1/1)`, its priority is `fee * min(1, 4) = fee * 1`. This means - /// that the transaction which consumes more resources (either length or weight) with the same - /// `fee` ends up having lower priority. + /// For Normal (and Mandatory) dispatchables we simply take the amount of `tip` payed + /// for every weight unit, scaled up by `T::WeightTipRatio`. + /// Operational dispatchables get an additional `priority` increase, roughly matching + /// a tip with amount equal to the cost required to include transaction that consumes + /// half of the block weight. fn get_priority( - len: usize, info: &DispatchInfoOf, - final_fee: BalanceOf, + tip: BalanceOf, ) -> TransactionPriority { - let weight_saturation = T::BlockWeights::get().max_block / info.weight.max(1); - let max_block_length = *T::BlockLength::get().max.get(DispatchClass::Normal); - let len_saturation = max_block_length as u64 / (len as u64).max(1); - let coefficient: BalanceOf = - weight_saturation.min(len_saturation).saturated_into::>(); - final_fee.saturating_mul(coefficient).saturated_into::() + let max_block = T::BlockWeights::get().max_block; + let bounded_weight = info.weight.max(1).min(max_block); + let weight_tip_ratio = T::WeightTipRatio::get(); + let per_weight = |val| FixedU128::saturating_from_rational(val, bounded_weight) + .saturating_mul_int(weight_tip_ratio); + + let tip_per_weight = per_weight(tip); + + match info.class { + DispatchClass::Normal => { + // For normal class we simply take the `tip_per_weight`. + tip_per_weight + }, + DispatchClass::Mandatory => { + // Mandatory extrinsics should be prohibited (e.g. by the [`CheckWeight`] extensions), + // but just to be safe let's return the same priority as `Normal` here. + tip_per_weight + }, + DispatchClass::Operational => { + // A reasonable tip value to frontrun an `Operational` extrinsic. + // This value should be kept high enough to allow `Operational` extrinsics + // from getting in even during congested period, but at the same time low + // enough to prevent a possible spam attack by sending invalid operational + // extrinsics which push away regular transactions from the pool. + // + // We assume that the reasonable tip is equal to the cost of transaction + // which fills up half of the block weight. + let half_block = max_block / 2; + let reasonable_tip = T::WeightToFee::calc(&half_block); + let reasonable_tip_per_weight = per_weight(reasonable_tip); + + tip_per_weight + .saturating_add(reasonable_tip_per_weight) + }, + }.saturated_into::() } } @@ -633,8 +676,9 @@ where info: &DispatchInfoOf, len: usize, ) -> TransactionValidity { - let (fee, _) = self.withdraw_fee(who, call, info, len)?; - Ok(ValidTransaction { priority: Self::get_priority(len, info, fee), ..Default::default() }) + let (_fee, _) = self.withdraw_fee(who, call, info, len)?; + let tip = self.0; + Ok(ValidTransaction { priority: Self::get_priority(info, tip), ..Default::default() }) } fn pre_dispatch( @@ -747,6 +791,7 @@ mod tests { pub const BlockHashCount: u64 = 250; pub static TransactionByteFee: u64 = 1; pub static WeightToFee: u64 = 1; + pub static WeightTipRatio: u64 = 1024 / 5; } impl frame_system::Config for Runtime { @@ -826,6 +871,7 @@ mod tests { impl Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = WeightToFee; type FeeMultiplierUpdate = (); } @@ -1339,6 +1385,59 @@ mod tests { }); } + + #[test] + fn should_alter_operational_priority() { + let tip = 5; + let len = 10; + + ExtBuilder::default() + .balance_factor(100) + .build() + .execute_with(|| { + let normal = DispatchInfo { + weight: 100, + class: DispatchClass::Normal, + pays_fee: Pays::Yes, + }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &normal, len) + .unwrap() + .priority; + + assert_eq!(priority, 10); + + let priority = ChargeTransactionPayment::(2 * tip) + .validate(&2, CALL, &normal, len) + .unwrap() + .priority; + + assert_eq!(priority, 20); + }); + + ExtBuilder::default() + .balance_factor(100) + .build() + .execute_with(|| { + let op = DispatchInfo { + weight: 100, + class: DispatchClass::Operational, + pays_fee: Pays::Yes, + }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, 1054); + + let priority = ChargeTransactionPayment::(2 * tip) + .validate(&2, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, 1064); + }); + } + #[test] fn post_info_can_change_pays_fee() { ExtBuilder::default() From b60f76dfdb4d613c746e91c577f7623542fbe8ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 21 Sep 2021 21:12:13 +0200 Subject: [PATCH 03/14] Fix tests. --- frame/balances/src/tests_composite.rs | 2 ++ frame/balances/src/tests_local.rs | 2 ++ frame/balances/src/tests_reentrancy.rs | 2 ++ frame/executive/src/lib.rs | 2 ++ 4 files changed, 8 insertions(+) diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs index f6faebed39316..f7318ce670be6 100644 --- a/frame/balances/src/tests_composite.rs +++ b/frame/balances/src/tests_composite.rs @@ -76,10 +76,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; + pub const WeightTipRatio: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index d8c07aa9c42e5..82f6bef50afd3 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -78,10 +78,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; + pub const WeightTipRatio: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_reentrancy.rs b/frame/balances/src/tests_reentrancy.rs index 9c7ba3e1ec824..430ea2a623e78 100644 --- a/frame/balances/src/tests_reentrancy.rs +++ b/frame/balances/src/tests_reentrancy.rs @@ -80,10 +80,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; + pub const WeightTipRatio: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 9541e94578c87..d14b0aa2cd90d 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -775,10 +775,12 @@ mod tests { parameter_types! { pub const TransactionByteFee: Balance = 0; + pub const WeightTipRatio: Balance = 1; } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; + type WeightTipRatio = WeightTipRatio; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } From a5420860ca7226a540a1f64591748167c221b51d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 22 Sep 2021 11:44:57 +0200 Subject: [PATCH 04/14] Update frame/transaction-payment/src/lib.rs Co-authored-by: Alexander Popiak --- frame/transaction-payment/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 29b04eb7aec12..e4d916ea76498 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -620,7 +620,7 @@ where DispatchClass::Operational => { // A reasonable tip value to frontrun an `Operational` extrinsic. // This value should be kept high enough to allow `Operational` extrinsics - // from getting in even during congested period, but at the same time low + // to get in even during congested period, but at the same time low // enough to prevent a possible spam attack by sending invalid operational // extrinsics which push away regular transactions from the pool. // From 7b598cbc784890cf527089393929cd0ebc5e1d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 22 Sep 2021 11:47:34 +0200 Subject: [PATCH 05/14] cargo +nightly fmt --all --- frame/system/src/extensions/check_weight.rs | 9 +- frame/transaction-payment/src/lib.rs | 95 +++++++++------------ 2 files changed, 44 insertions(+), 60 deletions(-) diff --git a/frame/system/src/extensions/check_weight.rs b/frame/system/src/extensions/check_weight.rs index 152d9f928883d..ca885accd660f 100644 --- a/frame/system/src/extensions/check_weight.rs +++ b/frame/system/src/extensions/check_weight.rs @@ -24,9 +24,7 @@ use frame_support::{ use scale_info::TypeInfo; use sp_runtime::{ traits::{DispatchInfoOf, Dispatchable, PostDispatchInfoOf, SignedExtension}, - transaction_validity::{ - InvalidTransaction, TransactionValidity, TransactionValidityError, - }, + transaction_validity::{InvalidTransaction, TransactionValidity, TransactionValidityError}, DispatchResult, }; @@ -355,10 +353,7 @@ mod tests { }; let len = 0_usize; - assert_eq!( - CheckWeight::::do_validate(&okay, len), - Ok(Default::default()) - ); + assert_eq!(CheckWeight::::do_validate(&okay, len), Ok(Default::default())); assert_err!( CheckWeight::::do_validate(&max, len), InvalidTransaction::ExhaustsResources diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index e4d916ea76498..2199cfdeb76f0 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -595,15 +595,14 @@ where /// Operational dispatchables get an additional `priority` increase, roughly matching /// a tip with amount equal to the cost required to include transaction that consumes /// half of the block weight. - fn get_priority( - info: &DispatchInfoOf, - tip: BalanceOf, - ) -> TransactionPriority { + fn get_priority(info: &DispatchInfoOf, tip: BalanceOf) -> TransactionPriority { let max_block = T::BlockWeights::get().max_block; let bounded_weight = info.weight.max(1).min(max_block); let weight_tip_ratio = T::WeightTipRatio::get(); - let per_weight = |val| FixedU128::saturating_from_rational(val, bounded_weight) - .saturating_mul_int(weight_tip_ratio); + let per_weight = |val| { + FixedU128::saturating_from_rational(val, bounded_weight) + .saturating_mul_int(weight_tip_ratio) + }; let tip_per_weight = per_weight(tip); @@ -613,8 +612,8 @@ where tip_per_weight }, DispatchClass::Mandatory => { - // Mandatory extrinsics should be prohibited (e.g. by the [`CheckWeight`] extensions), - // but just to be safe let's return the same priority as `Normal` here. + // Mandatory extrinsics should be prohibited (e.g. by the [`CheckWeight`] + // extensions), but just to be safe let's return the same priority as `Normal` here. tip_per_weight }, DispatchClass::Operational => { @@ -630,10 +629,10 @@ where let reasonable_tip = T::WeightToFee::calc(&half_block); let reasonable_tip_per_weight = per_weight(reasonable_tip); - tip_per_weight - .saturating_add(reasonable_tip_per_weight) + tip_per_weight.saturating_add(reasonable_tip_per_weight) }, - }.saturated_into::() + } + .saturated_into::() } } @@ -1385,57 +1384,47 @@ mod tests { }); } - #[test] fn should_alter_operational_priority() { let tip = 5; let len = 10; - ExtBuilder::default() - .balance_factor(100) - .build() - .execute_with(|| { - let normal = DispatchInfo { - weight: 100, - class: DispatchClass::Normal, - pays_fee: Pays::Yes, - }; - let priority = ChargeTransactionPayment::(tip) - .validate(&2, CALL, &normal, len) - .unwrap() - .priority; + ExtBuilder::default().balance_factor(100).build().execute_with(|| { + let normal = + DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &normal, len) + .unwrap() + .priority; - assert_eq!(priority, 10); + assert_eq!(priority, 10); - let priority = ChargeTransactionPayment::(2 * tip) - .validate(&2, CALL, &normal, len) - .unwrap() - .priority; + let priority = ChargeTransactionPayment::(2 * tip) + .validate(&2, CALL, &normal, len) + .unwrap() + .priority; - assert_eq!(priority, 20); - }); + assert_eq!(priority, 20); + }); - ExtBuilder::default() - .balance_factor(100) - .build() - .execute_with(|| { - let op = DispatchInfo { - weight: 100, - class: DispatchClass::Operational, - pays_fee: Pays::Yes, - }; - let priority = ChargeTransactionPayment::(tip) - .validate(&2, CALL, &op, len) - .unwrap() - .priority; - assert_eq!(priority, 1054); - - let priority = ChargeTransactionPayment::(2 * tip) - .validate(&2, CALL, &op, len) - .unwrap() - .priority; - assert_eq!(priority, 1064); - }); + ExtBuilder::default().balance_factor(100).build().execute_with(|| { + let op = DispatchInfo { + weight: 100, + class: DispatchClass::Operational, + pays_fee: Pays::Yes, + }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, 1054); + + let priority = ChargeTransactionPayment::(2 * tip) + .validate(&2, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, 1064); + }); } #[test] From b0d0c4852bfb98e17eff9364f1cd16608ffa0db3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Wed, 22 Sep 2021 11:53:58 +0200 Subject: [PATCH 06/14] Fix an obvious mistake :) --- bin/node/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 09a36e2ababb7..4ffd14117aa49 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -412,7 +412,7 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 10 * MILLICENTS; - pub const WeightTipRatio: Balance = MAXIMUM_BLOCK_WEIGHT as Balance / 1 * DOLLARS; + pub const WeightTipRatio: Balance = MAXIMUM_BLOCK_WEIGHT as Balance / DOLLARS; pub const TargetBlockFullness: Perquintill = Perquintill::from_percent(25); pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(1, 100_000); pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000_000u128); From 3b5f9f39c6ed14cfb5c33b8d96f1fdabd5616e1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 24 Sep 2021 15:17:09 +0200 Subject: [PATCH 07/14] Re-work again. --- bin/node-template/runtime/src/lib.rs | 4 +- bin/node/runtime/src/lib.rs | 4 +- frame/balances/src/tests_composite.rs | 4 +- frame/balances/src/tests_local.rs | 4 +- frame/balances/src/tests_reentrancy.rs | 4 +- frame/executive/src/lib.rs | 4 +- frame/transaction-payment/src/lib.rs | 131 ++++++++++++++++--------- 7 files changed, 96 insertions(+), 59 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index 3ffb3c0098d4d..977822d939565 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -257,13 +257,13 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 1; - pub WeightTipRatio: Balance = BlockWeights::get().max_block as Balance / ExistentialDeposit::get(); + pub OperationalVirtualTip: Balance = ExistentialDeposit::get(); } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 4ffd14117aa49..977d72f6d53cb 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -412,7 +412,7 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 10 * MILLICENTS; - pub const WeightTipRatio: Balance = MAXIMUM_BLOCK_WEIGHT as Balance / DOLLARS; + pub const OperationalVirtualTip: Balance = 1 * DOLLARS; pub const TargetBlockFullness: Perquintill = Perquintill::from_percent(25); pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(1, 100_000); pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000_000u128); @@ -421,7 +421,7 @@ parameter_types! { impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = TargetedFeeAdjustment; diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs index f7318ce670be6..80d9c00f5e10b 100644 --- a/frame/balances/src/tests_composite.rs +++ b/frame/balances/src/tests_composite.rs @@ -76,12 +76,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const WeightTipRatio: u64 = 1; + pub const OperationalVirtualTip: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index 82f6bef50afd3..3ef56e734a771 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -78,12 +78,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const WeightTipRatio: u64 = 1; + pub const OperationalVirtualTip: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_reentrancy.rs b/frame/balances/src/tests_reentrancy.rs index 430ea2a623e78..ec21631780712 100644 --- a/frame/balances/src/tests_reentrancy.rs +++ b/frame/balances/src/tests_reentrancy.rs @@ -80,12 +80,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const WeightTipRatio: u64 = 1; + pub const OperationalVirtualTip: u64 = 1; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index d14b0aa2cd90d..90308aa01ba8a 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -775,12 +775,12 @@ mod tests { parameter_types! { pub const TransactionByteFee: Balance = 0; - pub const WeightTipRatio: Balance = 1; + pub const OperationalVirtualTip: Balance = 1; } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 2199cfdeb76f0..47abf74606c19 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -264,16 +264,13 @@ pub mod pallet { #[pallet::constant] type TransactionByteFee: Get>; - /// The scaling factor for `priority` calculations. + /// A virtual (not payed) tip added to `Operational` extrinsics to boost their `priority`. /// - /// The priority is calculated by (integer) dividing the `tip` being payed and the amount of - /// `weight` the transaction consumes. Depending on the real-world values of both - /// `Balance` and `Weight` this division might end up in undesirable range. - /// To accomodated that, this additional scaling factor will be included in computations, - /// i.e. `tip * WeightTipRatio / weight`. - /// Recommended value for this constant is `MAXIMUM_BLOCK_WEIGHT / AVERAGE_TIP_AMOUNT`. + /// This value is simply included to a tip component in regular `priority` calculations. + /// It means that a `Normal` transaction can front-run a similarly-sized `Operational` + /// extrinsic (with no tip), by including a tip value greater than this. #[pallet::constant] - type WeightTipRatio: Get>; + type OperationalVirtualTip: Get>; /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: WeightToFeePolynomial>; @@ -583,53 +580,63 @@ where .map(|i| (fee, i)) } - /// Get an appropriate priority for a transaction with the given `DispatchInfo` and fee to be - /// paid (with `tip`). + /// Get an appropriate priority for a transaction with the given `DispatchInfo`, encoded length + /// and user-included tip. /// - /// Values returned by the function depend on the following: - /// 1. The average per-weight-unit tip to be payed. - /// 2. The dispatch class of the transaction. + /// The priority is based on the amount of `tip` the user is willing to pay per unit of either + /// `weight` or `length`, depending which one is more limitting. For `Operational` extrinsics + /// we add a "virtual tip" to the calculations. /// - /// For Normal (and Mandatory) dispatchables we simply take the amount of `tip` payed - /// for every weight unit, scaled up by `T::WeightTipRatio`. - /// Operational dispatchables get an additional `priority` increase, roughly matching - /// a tip with amount equal to the cost required to include transaction that consumes - /// half of the block weight. - fn get_priority(info: &DispatchInfoOf, tip: BalanceOf) -> TransactionPriority { - let max_block = T::BlockWeights::get().max_block; - let bounded_weight = info.weight.max(1).min(max_block); - let weight_tip_ratio = T::WeightTipRatio::get(); - let per_weight = |val| { - FixedU128::saturating_from_rational(val, bounded_weight) - .saturating_mul_int(weight_tip_ratio) - }; - - let tip_per_weight = per_weight(tip); + /// The formula should simply be `tip / bounded_{weight|length}`, but since we are using + /// integer division, we have no guarantees it's going to give results in any reasonable + /// range (might simply end up being zero). Hence we use a scaling factor: + /// `tip * (max_block_{weight|length} / bounded_{weight|length})`, since given current + /// state of-the-art blockchains, number of per-block transactions is expected to be in a + /// range reasonable enough to not saturate the `Balance` type while multiplying by the tip. + fn get_priority(info: &DispatchInfoOf, len: usize, tip: BalanceOf) -> TransactionPriority { + // Calculate how many such extrinsics we could fit into an empty block and take + // the limitting factor. + let max_block_weight = T::BlockWeights::get().max_block; + let max_block_length = *T::BlockLength::get().max.get(info.class) as u64; + + let bounded_weight = info.weight.max(1).min(max_block_weight); + let bounded_length = (len as u64).max(1).min(max_block_length); + + let max_tx_per_block_weight = max_block_weight / bounded_weight; + let max_tx_per_block_length = max_block_length / bounded_length; + // Given our current knowledge this value is going to be in a reasonable range - i.e. + // less than 10^9 (2^30), so multiplying by the `tip` value is unlikely to overflow the balance + // type. We still use saturating ops obviously, but the point is to end up with some + // `priority` distribution instead of having all transactions saturate the priority. + let max_tx_per_block = + max_tx_per_block_length.min(max_tx_per_block_weight).saturated_into::>(); + let max_reward = |val: BalanceOf| val.saturating_mul(max_tx_per_block); + + // To distribute no-tip transactions a little bit, we set the minimal tip as `1`. + // This means that given two transactions without a tip, smaller one will be preferred. + let tip = tip.max(1.saturated_into()); + let scaled_tip = max_reward(tip); match info.class { DispatchClass::Normal => { // For normal class we simply take the `tip_per_weight`. - tip_per_weight + scaled_tip }, DispatchClass::Mandatory => { // Mandatory extrinsics should be prohibited (e.g. by the [`CheckWeight`] // extensions), but just to be safe let's return the same priority as `Normal` here. - tip_per_weight + scaled_tip }, DispatchClass::Operational => { - // A reasonable tip value to frontrun an `Operational` extrinsic. + // A "virtual tip" value added to an `Operational` extrinsic. // This value should be kept high enough to allow `Operational` extrinsics - // to get in even during congested period, but at the same time low + // to get in even during congestion period, but at the same time low // enough to prevent a possible spam attack by sending invalid operational // extrinsics which push away regular transactions from the pool. - // - // We assume that the reasonable tip is equal to the cost of transaction - // which fills up half of the block weight. - let half_block = max_block / 2; - let reasonable_tip = T::WeightToFee::calc(&half_block); - let reasonable_tip_per_weight = per_weight(reasonable_tip); - - tip_per_weight.saturating_add(reasonable_tip_per_weight) + let virtual_tip = T::OperationalVirtualTip::get(); + let scaled_virtual_tip = max_reward(virtual_tip); + + scaled_tip.saturating_add(scaled_virtual_tip) }, } .saturated_into::() @@ -677,7 +684,7 @@ where ) -> TransactionValidity { let (_fee, _) = self.withdraw_fee(who, call, info, len)?; let tip = self.0; - Ok(ValidTransaction { priority: Self::get_priority(info, tip), ..Default::default() }) + Ok(ValidTransaction { priority: Self::get_priority(info, len, tip), ..Default::default() }) } fn pre_dispatch( @@ -790,7 +797,7 @@ mod tests { pub const BlockHashCount: u64 = 250; pub static TransactionByteFee: u64 = 1; pub static WeightToFee: u64 = 1; - pub static WeightTipRatio: u64 = 1024 / 5; + pub static OperationalVirtualTip: u64 = 5 * 5; } impl frame_system::Config for Runtime { @@ -870,7 +877,7 @@ mod tests { impl Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type WeightTipRatio = WeightTipRatio; + type OperationalVirtualTip = OperationalVirtualTip; type WeightToFee = WeightToFee; type FeeMultiplierUpdate = (); } @@ -1397,14 +1404,14 @@ mod tests { .unwrap() .priority; - assert_eq!(priority, 10); + assert_eq!(priority, 50); let priority = ChargeTransactionPayment::(2 * tip) .validate(&2, CALL, &normal, len) .unwrap() .priority; - assert_eq!(priority, 20); + assert_eq!(priority, 100); }); ExtBuilder::default().balance_factor(100).build().execute_with(|| { @@ -1417,13 +1424,43 @@ mod tests { .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 1054); + assert_eq!(priority, 300); let priority = ChargeTransactionPayment::(2 * tip) .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 1064); + assert_eq!(priority, 350); + }); + } + + #[test] + fn no_tip_has_some_priority() { + let tip = 0; + let len = 10; + + ExtBuilder::default().balance_factor(100).build().execute_with(|| { + let normal = + DispatchInfo { weight: 100, class: DispatchClass::Normal, pays_fee: Pays::Yes }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &normal, len) + .unwrap() + .priority; + + assert_eq!(priority, 50); + }); + + ExtBuilder::default().balance_factor(100).build().execute_with(|| { + let op = DispatchInfo { + weight: 100, + class: DispatchClass::Operational, + pays_fee: Pays::Yes, + }; + let priority = ChargeTransactionPayment::(tip) + .validate(&2, CALL, &op, len) + .unwrap() + .priority; + assert_eq!(priority, 300); }); } From 8372e5cb0ba176066393915f6d1449ff6d990c76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 24 Sep 2021 15:20:02 +0200 Subject: [PATCH 08/14] Fix test. --- frame/transaction-payment/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 47abf74606c19..df5db6f200d20 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -1447,7 +1447,7 @@ mod tests { .unwrap() .priority; - assert_eq!(priority, 50); + assert_eq!(priority, 10); }); ExtBuilder::default().balance_factor(100).build().execute_with(|| { @@ -1460,7 +1460,7 @@ mod tests { .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 300); + assert_eq!(priority, 260); }); } From c15151f294c33acb34e42c1e1b331fc34045b754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 24 Sep 2021 15:35:29 +0200 Subject: [PATCH 09/14] cargo +nightly fmt --all --- frame/transaction-payment/src/lib.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 5e298791ce761..a19947ee99f1a 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -595,7 +595,11 @@ where /// `tip * (max_block_{weight|length} / bounded_{weight|length})`, since given current /// state of-the-art blockchains, number of per-block transactions is expected to be in a /// range reasonable enough to not saturate the `Balance` type while multiplying by the tip. - fn get_priority(info: &DispatchInfoOf, len: usize, tip: BalanceOf) -> TransactionPriority { + fn get_priority( + info: &DispatchInfoOf, + len: usize, + tip: BalanceOf, + ) -> TransactionPriority { // Calculate how many such extrinsics we could fit into an empty block and take // the limitting factor. let max_block_weight = T::BlockWeights::get().max_block; @@ -607,11 +611,12 @@ where let max_tx_per_block_weight = max_block_weight / bounded_weight; let max_tx_per_block_length = max_block_length / bounded_length; // Given our current knowledge this value is going to be in a reasonable range - i.e. - // less than 10^9 (2^30), so multiplying by the `tip` value is unlikely to overflow the balance - // type. We still use saturating ops obviously, but the point is to end up with some + // less than 10^9 (2^30), so multiplying by the `tip` value is unlikely to overflow the + // balance type. We still use saturating ops obviously, but the point is to end up with some // `priority` distribution instead of having all transactions saturate the priority. - let max_tx_per_block = - max_tx_per_block_length.min(max_tx_per_block_weight).saturated_into::>(); + let max_tx_per_block = max_tx_per_block_length + .min(max_tx_per_block_weight) + .saturated_into::>(); let max_reward = |val: BalanceOf| val.saturating_mul(max_tx_per_block); // To distribute no-tip transactions a little bit, we set the minimal tip as `1`. From 75b132fde5f47d3076000a311c475d023b711b87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 27 Sep 2021 16:30:55 +0200 Subject: [PATCH 10/14] Make VirtualTip dependent on the transaction size. --- bin/node-template/runtime/src/lib.rs | 4 +-- bin/node/runtime/src/lib.rs | 5 ++-- frame/balances/src/tests_composite.rs | 4 +-- frame/balances/src/tests_local.rs | 4 +-- frame/balances/src/tests_reentrancy.rs | 4 +-- frame/executive/src/lib.rs | 4 +-- frame/transaction-payment/src/lib.rs | 38 +++++++++++++++++++------- 7 files changed, 41 insertions(+), 22 deletions(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index c563b3874ffdb..dcf511bc5cf6b 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -258,13 +258,13 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 1; - pub OperationalVirtualTip: Balance = ExistentialDeposit::get(); + pub OperationalFeeMultiplier: u8 = 5; } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplierBalance; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index ee6e1447ae15c..d84aa264e03f2 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -413,7 +413,7 @@ impl pallet_balances::Config for Runtime { parameter_types! { pub const TransactionByteFee: Balance = 10 * MILLICENTS; - pub const OperationalVirtualTip: Balance = 1 * DOLLARS; + pub const OperationalFeeMultiplier: u8 = 5; pub const TargetBlockFullness: Perquintill = Perquintill::from_percent(25); pub AdjustmentVariable: Multiplier = Multiplier::saturating_from_rational(1, 100_000); pub MinimumMultiplier: Multiplier = Multiplier::saturating_from_rational(1, 1_000_000_000u128); @@ -422,7 +422,8 @@ parameter_types! { impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; + type WeightToFee = IdentityFee; type FeeMultiplierUpdate = TargetedFeeAdjustment; diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs index 80d9c00f5e10b..60feedb326d8a 100644 --- a/frame/balances/src/tests_composite.rs +++ b/frame/balances/src/tests_composite.rs @@ -76,12 +76,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const OperationalVirtualTip: u64 = 1; + pub const OperationalFeeMultiplier: u8 = 5; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index 3ef56e734a771..1d758ce4e980b 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -78,12 +78,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const OperationalVirtualTip: u64 = 1; + pub const OperationalFeeMultiplier: u8 = 5; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/balances/src/tests_reentrancy.rs b/frame/balances/src/tests_reentrancy.rs index ec21631780712..25b8fb34f20ba 100644 --- a/frame/balances/src/tests_reentrancy.rs +++ b/frame/balances/src/tests_reentrancy.rs @@ -80,12 +80,12 @@ impl frame_system::Config for Test { } parameter_types! { pub const TransactionByteFee: u64 = 1; - pub const OperationalVirtualTip: u64 = 1; + pub const OperationalFeeMultiplier: u8 = 5; } impl pallet_transaction_payment::Config for Test { type OnChargeTransaction = CurrencyAdapter, ()>; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index a6ed46e36122c..b1bdf357ec07d 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -800,12 +800,12 @@ mod tests { parameter_types! { pub const TransactionByteFee: Balance = 0; - pub const OperationalVirtualTip: Balance = 1; + pub const OperationalFeeMultiplier: u8 = 5; } impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index a19947ee99f1a..ca1a96e2295e4 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -264,13 +264,29 @@ pub mod pallet { #[pallet::constant] type TransactionByteFee: Get>; - /// A virtual (not payed) tip added to `Operational` extrinsics to boost their `priority`. + /// A fee mulitplier for `Operational` extrinsics to compute "virtual tip" to boost their + /// `priority` /// - /// This value is simply included to a tip component in regular `priority` calculations. + /// This value is multipled by the `final_fee` to obtain a "virtual tip" that is later + /// added to a tip component in regular `priority` calculations. /// It means that a `Normal` transaction can front-run a similarly-sized `Operational` - /// extrinsic (with no tip), by including a tip value greater than this. + /// extrinsic (with no tip), by including a tip value greater than the virtual tip. + /// + /// ```rust,ignore + /// // For `Normal` + /// let priority = priority_calc(tip); + /// + /// // For `Operational` + /// let virtual_tip = (inclusion_fee + tip) * OperationalFeeMultiplier; + /// let priority = priority_calc(tip + virtual_tip); + /// ``` + /// + /// Note that since we use `final_fee` the multiplier applies also to the regular `tip` + /// sent with the transaction. So not only the transaction get's a priority bump based + /// on the `inclusion_fee` but also we amplify the impact of tips applied to `Operational` + /// transactions. #[pallet::constant] - type OperationalVirtualTip: Get>; + type OperationalFeeMultiplier: Get; /// Convert a weight value into a deductible fee based on the currency type. type WeightToFee: WeightToFeePolynomial>; @@ -595,10 +611,11 @@ where /// `tip * (max_block_{weight|length} / bounded_{weight|length})`, since given current /// state of-the-art blockchains, number of per-block transactions is expected to be in a /// range reasonable enough to not saturate the `Balance` type while multiplying by the tip. - fn get_priority( + pub fn get_priority( info: &DispatchInfoOf, len: usize, tip: BalanceOf, + final_fee: BalanceOf, ) -> TransactionPriority { // Calculate how many such extrinsics we could fit into an empty block and take // the limitting factor. @@ -640,7 +657,8 @@ where // to get in even during congestion period, but at the same time low // enough to prevent a possible spam attack by sending invalid operational // extrinsics which push away regular transactions from the pool. - let virtual_tip = T::OperationalVirtualTip::get(); + let fee_multiplier = T::OperationalFeeMultiplier::get().saturated_into(); + let virtual_tip = final_fee.saturating_mul(fee_multiplier); let scaled_virtual_tip = max_reward(virtual_tip); scaled_tip.saturating_add(scaled_virtual_tip) @@ -689,9 +707,9 @@ where info: &DispatchInfoOf, len: usize, ) -> TransactionValidity { - let (_fee, _) = self.withdraw_fee(who, call, info, len)?; + let (final_fee, _) = self.withdraw_fee(who, call, info, len)?; let tip = self.0; - Ok(ValidTransaction { priority: Self::get_priority(info, len, tip), ..Default::default() }) + Ok(ValidTransaction { priority: Self::get_priority(info, len, tip, final_fee), ..Default::default() }) } fn pre_dispatch( @@ -804,7 +822,7 @@ mod tests { pub const BlockHashCount: u64 = 250; pub static TransactionByteFee: u64 = 1; pub static WeightToFee: u64 = 1; - pub static OperationalVirtualTip: u64 = 5 * 5; + pub static OperationalFeeMultiplier: u8 = 5; } impl frame_system::Config for Runtime { @@ -884,7 +902,7 @@ mod tests { impl Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type OperationalVirtualTip = OperationalVirtualTip; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = WeightToFee; type FeeMultiplierUpdate = (); } From a7838fc2df949574f872c8184a12ec857fb0df4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 27 Sep 2021 16:43:37 +0200 Subject: [PATCH 11/14] cargo +nightly fmt --all --- frame/transaction-payment/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index ca1a96e2295e4..f670a9c9337dd 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -709,7 +709,10 @@ where ) -> TransactionValidity { let (final_fee, _) = self.withdraw_fee(who, call, info, len)?; let tip = self.0; - Ok(ValidTransaction { priority: Self::get_priority(info, len, tip, final_fee), ..Default::default() }) + Ok(ValidTransaction { + priority: Self::get_priority(info, len, tip, final_fee), + ..Default::default() + }) } fn pre_dispatch( @@ -1449,13 +1452,13 @@ mod tests { .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 300); + assert_eq!(priority, 5800); let priority = ChargeTransactionPayment::(2 * tip) .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 350); + assert_eq!(priority, 6100); }); } @@ -1485,7 +1488,7 @@ mod tests { .validate(&2, CALL, &op, len) .unwrap() .priority; - assert_eq!(priority, 260); + assert_eq!(priority, 5510); }); } From a317f73d0cdf3b8487c9db4b8fde9a31518aaf05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 30 Sep 2021 16:29:28 +0200 Subject: [PATCH 12/14] Update frame/transaction-payment/src/lib.rs Co-authored-by: Alexander Popiak --- frame/transaction-payment/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index f670a9c9337dd..28200bee7054f 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -282,8 +282,8 @@ pub mod pallet { /// ``` /// /// Note that since we use `final_fee` the multiplier applies also to the regular `tip` - /// sent with the transaction. So not only the transaction get's a priority bump based - /// on the `inclusion_fee` but also we amplify the impact of tips applied to `Operational` + /// sent with the transaction. So, not only does the transaction get a priority bump based + /// on the `inclusion_fee`, but we also amplify the impact of tips applied to `Operational` /// transactions. #[pallet::constant] type OperationalFeeMultiplier: Get; From 2f4276800144bd25ed6a078d576f3cd8362177b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 30 Sep 2021 16:41:32 +0200 Subject: [PATCH 13/14] Fix compilation. --- bin/node-template/runtime/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index dcf511bc5cf6b..5d3ea603265fe 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -264,7 +264,7 @@ parameter_types! { impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; - type OperationalFeeMultiplier = OperationalFeeMultiplierBalance; + type OperationalFeeMultiplier = OperationalFeeMultiplier; type WeightToFee = IdentityFee; type FeeMultiplierUpdate = (); } From 9fa9614b0eff36e57bd6e457b92d846817f954d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 1 Oct 2021 16:17:48 +0200 Subject: [PATCH 14/14] Update bin/node/runtime/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- bin/node/runtime/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d84aa264e03f2..419ba61cd7c49 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -423,7 +423,6 @@ impl pallet_transaction_payment::Config for Runtime { type OnChargeTransaction = CurrencyAdapter; type TransactionByteFee = TransactionByteFee; type OperationalFeeMultiplier = OperationalFeeMultiplier; - type WeightToFee = IdentityFee; type FeeMultiplierUpdate = TargetedFeeAdjustment;