Fix issues with Operational transactions validity and prioritization. (#6435)

* Fix weight limit for operational transactions.

* Include BlockExecutionWeight.

substrate/frame/system/src/lib.rs

@@ -1395,8 +1395,10 @@ impl<T: Trait + Send + Sync> CheckWeight<T> where
 		info: &DispatchInfoOf<T::Call>,
 	) -> Result<(), TransactionValidityError> {
 		match info.class {
-			// Mandatory and Operational transactions does not
+			// Mandatory transactions are included in a block unconditionally, so
-			DispatchClass::Mandatory | DispatchClass::Operational => Ok(()),
+			// we don't verify weight.
+			DispatchClass::Mandatory => Ok(()),
+			// Normal transactions must not exceed `MaximumExtrinsicWeight`.
 			DispatchClass::Normal => {
 				let maximum_weight = T::MaximumExtrinsicWeight::get();
 				let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get());
@@ -1405,7 +1407,22 @@ impl<T: Trait + Send + Sync> CheckWeight<T> where
 				} else {
 					Ok(())
 				}
-			}
+			},
+			// For operational transactions we make sure it doesn't exceed
+			// the space alloted for `Operational` class.
+			DispatchClass::Operational => {
+				let maximum_weight = T::MaximumBlockWeight::get();
+				let operational_limit =
+					Self::get_dispatch_limit_ratio(DispatchClass::Operational) * maximum_weight;
+				let operational_limit =
+					operational_limit.saturating_sub(T::BlockExecutionWeight::get());
+				let extrinsic_weight = info.weight.saturating_add(T::ExtrinsicBaseWeight::get());
+				if extrinsic_weight > operational_limit {
+					Err(InvalidTransaction::ExhaustsResources.into())
+				} else {
+					Ok(())
+				}
+			},
 		}
 	}
 
@@ -1484,9 +1501,11 @@ impl<T: Trait + Send + Sync> CheckWeight<T> where
 	fn get_priority(info: &DispatchInfoOf<T::Call>) -> TransactionPriority {
 		match info.class {
 			DispatchClass::Normal => info.weight.into(),
-			DispatchClass::Operational => Bounded::max_value(),
+			// 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,
 			// Mandatory extrinsics are only for inherents; never transactions.
-			DispatchClass::Mandatory => Bounded::min_value(),
+			DispatchClass::Mandatory => TransactionPriority::min_value(),
 		}
 	}
 
@@ -2452,6 +2471,42 @@ pub(crate) mod tests {
 		});
 	}
 
+	#[test]
+	fn operational_extrinsic_limited_by_operational_space_limit() {
+		new_test_ext().execute_with(|| {
+			let operational_limit = CheckWeight::<Test>::get_dispatch_limit_ratio(
+				DispatchClass::Operational
+			) * <Test as Trait>::MaximumBlockWeight::get();
+			let base_weight = <Test as Trait>::ExtrinsicBaseWeight::get();
+			let block_base = <Test as Trait>::BlockExecutionWeight::get();
+
+			let weight = operational_limit - base_weight - block_base;
+			let okay = DispatchInfo {
+				weight,
+				class: DispatchClass::Operational,
+				..Default::default()
+			};
+			let max = DispatchInfo {
+				weight: weight + 1,
+				class: DispatchClass::Operational,
+				..Default::default()
+			};
+			let len = 0_usize;
+
+			assert_eq!(
+				CheckWeight::<Test>::do_validate(&okay, len),
+				Ok(ValidTransaction {
+					priority: CheckWeight::<Test>::get_priority(&okay),
+					..Default::default()
+				})
+			);
+			assert_noop!(
+				CheckWeight::<Test>::do_validate(&max, len),
+				InvalidTransaction::ExhaustsResources
+			);
+		});
+	}
+
 	#[test]
 	fn register_extra_weight_unchecked_doesnt_care_about_limits() {
 		new_test_ext().execute_with(|| {
@@ -2479,6 +2534,8 @@ pub(crate) mod tests {
 			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&rest_operational, len));
 			assert_eq!(<Test as Trait>::MaximumBlockWeight::get(), 1024);
 			assert_eq!(System::block_weight().total(), <Test as Trait>::MaximumBlockWeight::get());
+			// Checking single extrinsic should not take current block weight into account.
+			assert_eq!(CheckWeight::<Test>::check_extrinsic_weight(&rest_operational), Ok(()));
 		});
 	}
 
@@ -2514,6 +2571,8 @@ pub(crate) mod tests {
 			assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&dispatch_operational, len));
 			// Not too much though
 			assert_noop!(CheckWeight::<Test>::do_pre_dispatch(&dispatch_operational, len), InvalidTransaction::ExhaustsResources);
+			// Even with full block, validity of single transaction should be correct.
+			assert_eq!(CheckWeight::<Test>::check_extrinsic_weight(&dispatch_operational), Ok(()));
 		});
 	}
 
@@ -2559,7 +2618,7 @@ pub(crate) mod tests {
 				.validate(&1, CALL, &op, len)
 				.unwrap()
 				.priority;
-			assert_eq!(priority, u64::max_value());
+			assert_eq!(priority, u64::max_value() / 2);
 		})
 	}