Rework Transaction Priority calculation (paritytech#9834)

* Add transaction validity docs.

* Re-work priority calculation.

* Fix tests.

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* cargo +nightly fmt --all

* Fix an obvious mistake :)

* Re-work again.

* Fix test.

* cargo +nightly fmt --all

* Make VirtualTip dependent on the transaction size.

* cargo +nightly fmt --all

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

* Fix compilation.

* Update bin/node/runtime/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

bin/node-template/runtime/src/lib.rs

@@ -258,11 +258,13 @@ impl pallet_balances::Config for Runtime {
 
 parameter_types! {
 	pub const TransactionByteFee: Balance = 1;
+	pub OperationalFeeMultiplier: u8 = 5;
 }
 
 impl pallet_transaction_payment::Config for Runtime {
 	type OnChargeTransaction = CurrencyAdapter<Balances, ()>;
 	type TransactionByteFee = TransactionByteFee;
+	type OperationalFeeMultiplier = OperationalFeeMultiplier;
 	type WeightToFee = IdentityFee<Balance>;
 	type FeeMultiplierUpdate = ();
 }

bin/node/runtime/src/lib.rs

@@ -413,6 +413,7 @@ impl pallet_balances::Config for Runtime {
 
 parameter_types! {
 	pub const TransactionByteFee: Balance = 10 * MILLICENTS;
+	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);
@@ -421,6 +422,7 @@ parameter_types! {
 impl pallet_transaction_payment::Config for Runtime {
 	type OnChargeTransaction = CurrencyAdapter<Balances, DealWithFees>;
 	type TransactionByteFee = TransactionByteFee;
+	type OperationalFeeMultiplier = OperationalFeeMultiplier;
 	type WeightToFee = IdentityFee<Balance>;
 	type FeeMultiplierUpdate =
 		TargetedFeeAdjustment<Self, TargetBlockFullness, AdjustmentVariable, MinimumMultiplier>;

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 OperationalFeeMultiplier: u8 = 5;
 }
 impl pallet_transaction_payment::Config for Test {
 	type OnChargeTransaction = CurrencyAdapter<Pallet<Test>, ()>;
 	type TransactionByteFee = TransactionByteFee;
+	type OperationalFeeMultiplier = OperationalFeeMultiplier;
 	type WeightToFee = IdentityFee<u64>;
 	type FeeMultiplierUpdate = ();
 }

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 OperationalFeeMultiplier: u8 = 5;
 }
 impl pallet_transaction_payment::Config for Test {
 	type OnChargeTransaction = CurrencyAdapter<Pallet<Test>, ()>;
 	type TransactionByteFee = TransactionByteFee;
+	type OperationalFeeMultiplier = OperationalFeeMultiplier;
 	type WeightToFee = IdentityFee<u64>;
 	type FeeMultiplierUpdate = ();
 }

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 OperationalFeeMultiplier: u8 = 5;
 }
 impl pallet_transaction_payment::Config for Test {
 	type OnChargeTransaction = CurrencyAdapter<Pallet<Test>, ()>;
 	type TransactionByteFee = TransactionByteFee;
+	type OperationalFeeMultiplier = OperationalFeeMultiplier;
 	type WeightToFee = IdentityFee<u64>;
 	type FeeMultiplierUpdate = ();
 }

frame/executive/src/lib.rs

@@ -800,10 +800,12 @@ mod tests {
 
 	parameter_types! {
 		pub const TransactionByteFee: Balance = 0;
+		pub const OperationalFeeMultiplier: u8 = 5;
 	}
 	impl pallet_transaction_payment::Config for Runtime {
 		type OnChargeTransaction = CurrencyAdapter<Balances, ()>;
 		type TransactionByteFee = TransactionByteFee;
+		type OperationalFeeMultiplier = OperationalFeeMultiplier;
 		type WeightToFee = IdentityFee<Balance>;
 		type FeeMultiplierUpdate = ();
 	}
@@ -1110,16 +1112,14 @@ 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(
 					TransactionSource::InBlock,
 					valid.clone(),
 					Default::default(),
 				),
-				Ok(default_with_prio_3),
+				Ok(ValidTransaction::default()),
 			);
 			assert_eq!(
 				Executive::validate_transaction(

frame/support/src/weights.rs

@@ -287,30 +287,6 @@ impl<'a> OneOrMany<DispatchClass> 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<FrameTransactionPriority> 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 {

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<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>);

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<T: Config + Send + Sync>(Era, sp_std::marker::PhantomData<T>);

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<T: Config>(#[codec(compact)] pub T::Index);

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<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>);

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<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>);

frame/system/src/extensions/check_weight.rs

@@ -19,19 +19,21 @@ 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,
-	},
+	transaction_validity::{InvalidTransaction, TransactionValidity, TransactionValidityError},
 	DispatchResult,
 };
 
 /// Block resource (weight) limit check.
+///
+/// # Transaction Validity
+///
+/// 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<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>);
@@ -81,23 +83,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<T::Call>) -> 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())
@@ -130,7 +115,7 @@ where
 		// consumption from causing false negatives.
 		Self::check_extrinsic_weight(info)?;
 
-		Ok(ValidTransaction { priority: Self::get_priority(info), ..Default::default() })
+		Ok(Default::default())
 	}
 }
 
@@ -368,13 +353,7 @@ mod tests {
 			};
 			let len = 0_usize;
 
-			assert_eq!(
-				CheckWeight::<Test>::do_validate(&okay, len),
-				Ok(ValidTransaction {
-					priority: CheckWeight::<Test>::get_priority(&okay),
-					..Default::default()
-				})
-			);
+			assert_eq!(CheckWeight::<Test>::do_validate(&okay, len), Ok(Default::default()));
 			assert_err!(
 				CheckWeight::<Test>::do_validate(&max, len),
 				InvalidTransaction::ExhaustsResources
@@ -506,30 +485,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::<Test>(PhantomData)
-				.validate(&1, CALL, &normal, len)
-				.unwrap()
-				.priority;
-			assert_eq!(priority, 100);
-
-			let priority =
-				CheckWeight::<Test>(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(|| {