Correct arithmetical semantic of PerDispatchClass (#13194)

* Fix PerDispatchClass arithmetic

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Test PerDispatchClass arithmetic

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add helpers for Weight

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove stale doc

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Dont mention Polkadot in Substrate

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fmt

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Remove saturating_ prefix

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix usage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

substrate/frame/support/src/dispatch.rs

@@ -423,33 +423,35 @@ impl<T: Clone> PerDispatchClass<T> {
 
 impl PerDispatchClass<Weight> {
 	/// Returns the total weight consumed by all extrinsics in the block.
+	///
+	/// Saturates on overflow.
 	pub fn total(&self) -> Weight {
 		let mut sum = Weight::zero();
 		for class in DispatchClass::all() {
-			sum = sum.saturating_add(*self.get(*class));
+			sum.saturating_accrue(*self.get(*class));
 		}
 		sum
 	}
 
-	/// Add some weight of a specific dispatch class, saturating at the numeric bounds of `Weight`.
-	pub fn add(&mut self, weight: Weight, class: DispatchClass) {
-		let value = self.get_mut(class);
-		*value = value.saturating_add(weight);
+	/// Add some weight to the given class. Saturates at the numeric bounds.
+	pub fn add(mut self, weight: Weight, class: DispatchClass) -> Self {
+		self.accrue(weight, class);
+		self
 	}
 
-	/// Try to add some weight of a specific dispatch class, returning Err(()) if overflow would
-	/// occur.
-	pub fn checked_add(&mut self, weight: Weight, class: DispatchClass) -> Result<(), ()> {
-		let value = self.get_mut(class);
-		*value = value.checked_add(&weight).ok_or(())?;
-		Ok(())
+	/// Increase the weight of the given class. Saturates at the numeric bounds.
+	pub fn accrue(&mut self, weight: Weight, class: DispatchClass) {
+		self.get_mut(class).saturating_accrue(weight);
 	}
 
-	/// Subtract some weight of a specific dispatch class, saturating at the numeric bounds of
-	/// `Weight`.
-	pub fn sub(&mut self, weight: Weight, class: DispatchClass) {
-		let value = self.get_mut(class);
-		*value = value.saturating_sub(weight);
+	/// Try to increase the weight of the given class. Saturates at the numeric bounds.
+	pub fn checked_accrue(&mut self, weight: Weight, class: DispatchClass) -> Result<(), ()> {
+		self.get_mut(class).checked_accrue(weight).ok_or(())
+	}
+
+	/// Reduce the weight of the given class. Saturates at the numeric bounds.
+	pub fn reduce(&mut self, weight: Weight, class: DispatchClass) {
+		self.get_mut(class).saturating_reduce(weight);
 	}
 }
 
@@ -3693,3 +3695,178 @@ mod weight_tests {
 		assert_eq!(extract_actual_pays_fee(&Ok((Some(1000), Pays::Yes).into()), &pre), Pays::No);
 	}
 }
+
+#[cfg(test)]
+mod per_dispatch_class_tests {
+	use super::*;
+	use sp_runtime::traits::Zero;
+	use DispatchClass::*;
+
+	#[test]
+	fn add_works() {
+		let a = PerDispatchClass {
+			normal: (5, 10).into(),
+			operational: (20, 30).into(),
+			mandatory: Weight::MAX,
+		};
+		assert_eq!(
+			a.clone()
+				.add((20, 5).into(), Normal)
+				.add((10, 10).into(), Operational)
+				.add((u64::MAX, 3).into(), Mandatory),
+			PerDispatchClass {
+				normal: (25, 15).into(),
+				operational: (30, 40).into(),
+				mandatory: Weight::MAX
+			}
+		);
+		let b = a
+			.add(Weight::MAX, Normal)
+			.add(Weight::MAX, Operational)
+			.add(Weight::MAX, Mandatory);
+		assert_eq!(
+			b,
+			PerDispatchClass {
+				normal: Weight::MAX,
+				operational: Weight::MAX,
+				mandatory: Weight::MAX
+			}
+		);
+		assert_eq!(b.total(), Weight::MAX);
+	}
+
+	#[test]
+	fn accrue_works() {
+		let mut a = PerDispatchClass::default();
+
+		a.accrue((10, 15).into(), Normal);
+		assert_eq!(a.normal, (10, 15).into());
+		assert_eq!(a.total(), (10, 15).into());
+
+		a.accrue((20, 25).into(), Operational);
+		assert_eq!(a.operational, (20, 25).into());
+		assert_eq!(a.total(), (30, 40).into());
+
+		a.accrue((30, 35).into(), Mandatory);
+		assert_eq!(a.mandatory, (30, 35).into());
+		assert_eq!(a.total(), (60, 75).into());
+
+		a.accrue((u64::MAX, 10).into(), Operational);
+		assert_eq!(a.operational, (u64::MAX, 35).into());
+		assert_eq!(a.total(), (u64::MAX, 85).into());
+
+		a.accrue((10, u64::MAX).into(), Normal);
+		assert_eq!(a.normal, (20, u64::MAX).into());
+		assert_eq!(a.total(), Weight::MAX);
+	}
+
+	#[test]
+	fn reduce_works() {
+		let mut a = PerDispatchClass {
+			normal: (10, u64::MAX).into(),
+			mandatory: (u64::MAX, 10).into(),
+			operational: (20, 20).into(),
+		};
+
+		a.reduce((5, 100).into(), Normal);
+		assert_eq!(a.normal, (5, u64::MAX - 100).into());
+		assert_eq!(a.total(), (u64::MAX, u64::MAX - 70).into());
+
+		a.reduce((15, 5).into(), Operational);
+		assert_eq!(a.operational, (5, 15).into());
+		assert_eq!(a.total(), (u64::MAX, u64::MAX - 75).into());
+
+		a.reduce((50, 0).into(), Mandatory);
+		assert_eq!(a.mandatory, (u64::MAX - 50, 10).into());
+		assert_eq!(a.total(), (u64::MAX - 40, u64::MAX - 75).into());
+
+		a.reduce((u64::MAX, 100).into(), Operational);
+		assert!(a.operational.is_zero());
+		assert_eq!(a.total(), (u64::MAX - 45, u64::MAX - 90).into());
+
+		a.reduce((5, u64::MAX).into(), Normal);
+		assert!(a.normal.is_zero());
+		assert_eq!(a.total(), (u64::MAX - 50, 10).into());
+	}
+
+	#[test]
+	fn checked_accrue_works() {
+		let mut a = PerDispatchClass::default();
+
+		a.checked_accrue((1, 2).into(), Normal).unwrap();
+		a.checked_accrue((3, 4).into(), Operational).unwrap();
+		a.checked_accrue((5, 6).into(), Mandatory).unwrap();
+		a.checked_accrue((7, 8).into(), Operational).unwrap();
+		a.checked_accrue((9, 0).into(), Normal).unwrap();
+
+		assert_eq!(
+			a,
+			PerDispatchClass {
+				normal: (10, 2).into(),
+				operational: (10, 12).into(),
+				mandatory: (5, 6).into(),
+			}
+		);
+
+		a.checked_accrue((u64::MAX - 10, u64::MAX - 2).into(), Normal).unwrap();
+		a.checked_accrue((0, 0).into(), Normal).unwrap();
+		a.checked_accrue((1, 0).into(), Normal).unwrap_err();
+		a.checked_accrue((0, 1).into(), Normal).unwrap_err();
+
+		assert_eq!(
+			a,
+			PerDispatchClass {
+				normal: Weight::MAX,
+				operational: (10, 12).into(),
+				mandatory: (5, 6).into(),
+			}
+		);
+	}
+
+	#[test]
+	fn checked_accrue_does_not_modify_on_error() {
+		let mut a = PerDispatchClass {
+			normal: 0.into(),
+			operational: Weight::MAX / 2 + 2.into(),
+			mandatory: 10.into(),
+		};
+
+		a.checked_accrue(Weight::MAX / 2, Operational).unwrap_err();
+		a.checked_accrue(Weight::MAX - 9.into(), Mandatory).unwrap_err();
+		a.checked_accrue(Weight::MAX, Normal).unwrap(); // This one works
+
+		assert_eq!(
+			a,
+			PerDispatchClass {
+				normal: Weight::MAX,
+				operational: Weight::MAX / 2 + 2.into(),
+				mandatory: 10.into(),
+			}
+		);
+	}
+
+	#[test]
+	fn total_works() {
+		assert!(PerDispatchClass::default().total().is_zero());
+
+		assert_eq!(
+			PerDispatchClass {
+				normal: 0.into(),
+				operational: (10, 20).into(),
+				mandatory: (20, u64::MAX).into(),
+			}
+			.total(),
+			(30, u64::MAX).into()
+		);
+
+		assert_eq!(
+			PerDispatchClass {
+				normal: (u64::MAX - 10, 10).into(),
+				operational: (3, u64::MAX).into(),
+				mandatory: (4, u64::MAX).into(),
+			}
+			.total(),
+			(u64::MAX - 3, u64::MAX).into()
+		);
+	}
+}

substrate/frame/support/src/weights.rs

@@ -15,15 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//! Re-exports `sp-weights` public API, and contains benchmarked weight constants specific to
-//! FRAME.
-//!
-//! Latest machine specification used to benchmark are:
-//! - Digital Ocean: ubuntu-s-2vcpu-4gb-ams3-01
-//! - 2x Intel(R) Xeon(R) CPU E5-2650 v4 @ 2.20GHz
-//! - 4GB RAM
-//! - Ubuntu 19.10 (GNU/Linux 5.3.0-18-generic x86_64)
-//! - rustc 1.42.0 (b8cedc004 2020-03-09)
+//! Re-exports `sp-weights` public API, and contains benchmarked weight constants specific to FRAME.
 
 mod block_weights;
 mod extrinsic_weights;

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

@@ -135,10 +135,10 @@ where
 
 	// add the weight. If class is unlimited, use saturating add instead of checked one.
 	if limit_per_class.max_total.is_none() && limit_per_class.reserved.is_none() {
-		all_weight.add(extrinsic_weight, info.class)
+		all_weight.accrue(extrinsic_weight, info.class)
 	} else {
 		all_weight
-			.checked_add(extrinsic_weight, info.class)
+			.checked_accrue(extrinsic_weight, info.class)
 			.map_err(|_| InvalidTransaction::ExhaustsResources)?;
 	}
 
@@ -229,7 +229,7 @@ where
 		let unspent = post_info.calc_unspent(info);
 		if unspent.any_gt(Weight::zero()) {
 			crate::BlockWeight::<T>::mutate(|current_weight| {
-				current_weight.sub(unspent, info.class);
+				current_weight.reduce(unspent, info.class);
 			})
 		}
 

substrate/frame/system/src/lib.rs

@@ -1321,7 +1321,7 @@ impl<T: Config> Pallet<T> {
 	/// Another potential use-case could be for the `on_initialize` and `on_finalize` hooks.
 	pub fn register_extra_weight_unchecked(weight: Weight, class: DispatchClass) {
 		BlockWeight::<T>::mutate(|current_weight| {
-			current_weight.add(weight, class);
+			current_weight.accrue(weight, class);
 		});
 	}
 

substrate/primitives/weights/src/lib.rs

@@ -16,13 +16,6 @@
 // limitations under the License.
 
 //! # Primitives for transaction weighting.
-//!
-//! Latest machine specification used to benchmark are:
-//! - Digital Ocean: ubuntu-s-2vcpu-4gb-ams3-01
-//! - 2x Intel(R) Xeon(R) CPU E5-2650 v4 @ 2.20GHz
-//! - 4GB RAM
-//! - Ubuntu 19.10 (GNU/Linux 5.3.0-18-generic x86_64)
-//! - rustc 1.42.0 (b8cedc004 2020-03-09)
 
 #![cfg_attr(not(feature = "std"), no_std)]
 

substrate/primitives/weights/src/weight_v2.rs

@@ -117,6 +117,11 @@ impl Weight {
 		Self { ref_time, proof_size }
 	}
 
+	/// Construct [`Weight`] from the same weight for all parts.
+	pub const fn from_all(value: u64) -> Self {
+		Self { ref_time: value, proof_size: value }
+	}
+
 	/// Saturating [`Weight`] addition. Computes `self + rhs`, saturating at the numeric bounds of
 	/// all fields instead of overflowing.
 	pub const fn saturating_add(self, rhs: Self) -> Self {
@@ -167,6 +172,11 @@ impl Weight {
 		*self = self.saturating_add(amount);
 	}
 
+	/// Reduce [`Weight`] by `amount` via saturating subtraction.
+	pub fn saturating_reduce(&mut self, amount: Self) {
+		*self = self.saturating_sub(amount);
+	}
+
 	/// Checked [`Weight`] addition. Computes `self + rhs`, returning `None` if overflow occurred.
 	pub const fn checked_add(&self, rhs: &Self) -> Option<Self> {
 		let ref_time = match self.ref_time.checked_add(rhs.ref_time) {
@@ -222,6 +232,16 @@ impl Weight {
 		Some(Self { ref_time, proof_size })
 	}
 
+	/// Try to increase `self` by `amount` via checked addition.
+	pub fn checked_accrue(&mut self, amount: Self) -> Option<()> {
+		self.checked_add(&amount).map(|new_self| *self = new_self)
+	}
+
+	/// Try to reduce `self` by `amount` via checked subtraction.
+	pub fn checked_reduce(&mut self, amount: Self) -> Option<()> {
+		self.checked_sub(&amount).map(|new_self| *self = new_self)
+	}
+
 	/// Return a [`Weight`] where all fields are zero.
 	pub const fn zero() -> Self {
 		Self { ref_time: 0, proof_size: 0 }
@@ -352,6 +372,20 @@ where
 	}
 }
 
+#[cfg(any(test, feature = "std", feature = "runtime-benchmarks"))]
+impl From<u64> for Weight {
+	fn from(value: u64) -> Self {
+		Self::from_parts(value, value)
+	}
+}
+
+#[cfg(any(test, feature = "std", feature = "runtime-benchmarks"))]
+impl From<(u64, u64)> for Weight {
+	fn from(value: (u64, u64)) -> Self {
+		Self::from_parts(value.0, value.1)
+	}
+}
+
 macro_rules! weight_mul_per_impl {
 	($($t:ty),* $(,)?) => {
 		$(
@@ -459,4 +493,79 @@ mod tests {
 		assert!(!Weight::from_parts(0, 1).is_zero());
 		assert!(!Weight::MAX.is_zero());
 	}
+
+	#[test]
+	fn from_parts_works() {
+		assert_eq!(Weight::from_parts(0, 0), Weight { ref_time: 0, proof_size: 0 });
+		assert_eq!(Weight::from_parts(5, 5), Weight { ref_time: 5, proof_size: 5 });
+		assert_eq!(
+			Weight::from_parts(u64::MAX, u64::MAX),
+			Weight { ref_time: u64::MAX, proof_size: u64::MAX }
+		);
+	}
+
+	#[test]
+	fn from_all_works() {
+		assert_eq!(Weight::from_all(0), Weight::from_parts(0, 0));
+		assert_eq!(Weight::from_all(5), Weight::from_parts(5, 5));
+		assert_eq!(Weight::from_all(u64::MAX), Weight::from_parts(u64::MAX, u64::MAX));
+	}
+
+	#[test]
+	fn from_u64_works() {
+		assert_eq!(Weight::from_all(0), 0_u64.into());
+		assert_eq!(Weight::from_all(123), 123_u64.into());
+		assert_eq!(Weight::from_all(u64::MAX), u64::MAX.into());
+	}
+
+	#[test]
+	fn from_u64_pair_works() {
+		assert_eq!(Weight::from_parts(0, 1), (0, 1).into());
+		assert_eq!(Weight::from_parts(123, 321), (123u64, 321u64).into());
+		assert_eq!(Weight::from_parts(u64::MAX, 0), (u64::MAX, 0).into());
+	}
+
+	#[test]
+	fn saturating_reduce_works() {
+		let mut weight = Weight::from_parts(10, 20);
+		weight.saturating_reduce(Weight::from_all(5));
+		assert_eq!(weight, Weight::from_parts(5, 15));
+		weight.saturating_reduce(Weight::from_all(5));
+		assert_eq!(weight, Weight::from_parts(0, 10));
+		weight.saturating_reduce(Weight::from_all(11));
+		assert!(weight.is_zero());
+		weight.saturating_reduce(Weight::from_all(u64::MAX));
+		assert!(weight.is_zero());
+	}
+
+	#[test]
+	fn checked_accrue_works() {
+		let mut weight = Weight::from_parts(10, 20);
+		assert!(weight.checked_accrue(Weight::from_all(2)).is_some());
+		assert_eq!(weight, Weight::from_parts(12, 22));
+		assert!(weight.checked_accrue(Weight::from_parts(u64::MAX, 0)).is_none());
+		assert!(weight.checked_accrue(Weight::from_parts(0, u64::MAX)).is_none());
+		assert_eq!(weight, Weight::from_parts(12, 22));
+		assert!(weight
+			.checked_accrue(Weight::from_parts(u64::MAX - 12, u64::MAX - 22))
+			.is_some());
+		assert_eq!(weight, Weight::MAX);
+		assert!(weight.checked_accrue(Weight::from_parts(1, 0)).is_none());
+		assert!(weight.checked_accrue(Weight::from_parts(0, 1)).is_none());
+		assert_eq!(weight, Weight::MAX);
+	}
+
+	#[test]
+	fn checked_reduce_works() {
+		let mut weight = Weight::from_parts(10, 20);
+		assert!(weight.checked_reduce(Weight::from_all(2)).is_some());
+		assert_eq!(weight, Weight::from_parts(8, 18));
+		assert!(weight.checked_reduce(Weight::from_parts(9, 0)).is_none());
+		assert!(weight.checked_reduce(Weight::from_parts(0, 19)).is_none());
+		assert_eq!(weight, Weight::from_parts(8, 18));
+		assert!(weight.checked_reduce(Weight::from_parts(8, 0)).is_some());
+		assert_eq!(weight, Weight::from_parts(0, 18));
+		assert!(weight.checked_reduce(Weight::from_parts(0, 18)).is_some());
+		assert!(weight.is_zero());
+	}
 }