Partial fix for transaction priority (#7034)

* Partial fix for priority stuff.

* Small fix

* Fix tests.

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

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* Better doc

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

substrate/frame/support/src/weights.rs

@@ -242,6 +242,30 @@ impl Default for DispatchClass {
 	}
 }
 
+/// Primitives related to priority management of Frame.
+pub mod priority {
+	/// The starting point of all Operational transactions. 3/4 of u64::max_value().
+	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)]
 pub struct DispatchInfo {

substrate/frame/system/src/extensions/check_nonce.rs

@@ -25,12 +25,15 @@ use sp_runtime::{
 	traits::{SignedExtension, DispatchInfoOf, Dispatchable, One},
 	transaction_validity::{
 		ValidTransaction, TransactionValidityError, InvalidTransaction, TransactionValidity,
-		TransactionLongevity, TransactionPriority,
+		TransactionLongevity,
 	},
 };
 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.
 #[derive(Encode, Decode, Clone, Eq, PartialEq)]
 pub struct CheckNonce<T: Trait>(#[codec(compact)] T::Index);
 
@@ -90,7 +93,7 @@ impl<T: Trait> SignedExtension for CheckNonce<T> where
 		&self,
 		who: &Self::AccountId,
 		_call: &Self::Call,
-		info: &DispatchInfoOf<Self::Call>,
+		_info: &DispatchInfoOf<Self::Call>,
 		_len: usize,
 	) -> TransactionValidity {
 		// check index
@@ -107,7 +110,7 @@ impl<T: Trait> SignedExtension for CheckNonce<T> where
 		};
 
 		Ok(ValidTransaction {
-			priority: info.weight as TransactionPriority,
+			priority: 0,
 			requires,
 			provides,
 			longevity: TransactionLongevity::max_value(),

substrate/frame/system/src/extensions/check_weight.rs

@@ -27,7 +27,7 @@ use sp_runtime::{
 };
 use frame_support::{
 	traits::{Get},
-	weights::{PostDispatchInfo, DispatchInfo, DispatchClass},
+	weights::{PostDispatchInfo, DispatchInfo, DispatchClass, priority::FrameTransactionPriority},
 	StorageValue,
 };
 
@@ -157,12 +157,18 @@ impl<T: Trait + Send + Sync> CheckWeight<T> 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 {
-			DispatchClass::Normal => info.weight.into(),
-			// Don't use up the whole priority space, to allow things like `tip`
-			// to be taken into account as well.
-			DispatchClass::Operational => TransactionPriority::max_value() / 2,
+			// 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(),
 		}
@@ -496,7 +502,7 @@ mod tests {
 	}
 
 	#[test]
-	fn signed_ext() {
+	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 };
@@ -512,7 +518,7 @@ mod tests {
 				.validate(&1, CALL, &op, len)
 				.unwrap()
 				.priority;
-			assert_eq!(priority, u64::max_value() / 2);
+			assert_eq!(priority, frame_support::weights::priority::LIMIT + 100);
 		})
 	}
 

substrate/frame/transaction-payment/src/lib.rs

@@ -467,6 +467,23 @@ impl<T: Trait + Send + Sync> ChargeTransactionPayment<T> where
 			Err(_) => Err(InvalidTransaction::Payment.into()),
 		}
 	}
+
+	/// Get an appropriate priority for a transaction with the given length and info.
+	///
+	/// This will try and optimise the `fee/weight` `fee/length`, whichever is consuming more of the
+	/// maximum corresponding limit.
+	///
+	/// 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.
+	fn get_priority(len: usize, info: &DispatchInfoOf<T::Call>, final_fee: BalanceOf<T>) -> TransactionPriority {
+		let weight_saturation = T::MaximumBlockWeight::get() / info.weight.max(1);
+		let len_saturation = T::MaximumBlockLength::get() as u64 / (len as u64).max(1);
+		let coefficient: BalanceOf<T> = weight_saturation.min(len_saturation).saturated_into::<BalanceOf<T>>();
+		final_fee.saturating_mul(coefficient).saturated_into::<TransactionPriority>()
+	}
 }
 
 impl<T: Trait + Send + Sync> sp_std::fmt::Debug for ChargeTransactionPayment<T> {
@@ -499,12 +516,10 @@ impl<T: Trait + Send + Sync> SignedExtension for ChargeTransactionPayment<T> whe
 		len: usize,
 	) -> TransactionValidity {
 		let (fee, _) = self.withdraw_fee(who, info, len)?;
-
-		let mut r = ValidTransaction::default();
-		// NOTE: we probably want to maximize the _fee (of any type) per weight unit_ here, which
-		// will be a bit more than setting the priority to tip. For now, this is enough.
-		r.priority = fee.saturated_into::<TransactionPriority>();
-		Ok(r)
+		Ok(ValidTransaction {
+			priority: Self::get_priority(len, info, fee),
+			..Default::default()
+		})
 	}
 
 	fn pre_dispatch(