Update Balances Pallet to use WeightInfo (#6610)

* Update balance benchmarks

* Update weight functions

* Remove user component

* make componentless

* Add support for `#[extra]` tag on benchmarks

* Update balances completely

* Apply suggestions from code review

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

* Fix some tests

* Maybe fix to test. Need approval from @tomusdrw this is okay

* Make test better

* keep weights conservative

* Update macro for merge master

* Add headers

* Apply suggestions from code review

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

Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

substrate/frame/balances/src/benchmarking.rs

@@ -28,46 +28,40 @@ use sp_runtime::traits::Bounded;
 use crate::Module as Balances;
 
 const SEED: u32 = 0;
-const MAX_EXISTENTIAL_DEPOSIT: u32 = 1000;
-const MAX_USER_INDEX: u32 = 1000;
+// existential deposit multiplier
+const ED_MULTIPLIER: u32 = 10;
+
 
 benchmarks! {
-	_ {
-		let e in 2 .. MAX_EXISTENTIAL_DEPOSIT => ();
-		let u in 1 .. MAX_USER_INDEX => ();
-	}
+	_ { }
 
 	// Benchmark `transfer` extrinsic with the worst possible conditions:
 	// * Transfer will kill the sender account.
 	// * Transfer will create the recipient account.
 	transfer {
-		let u in ...;
-		let e in ...;
-
 		let existential_deposit = T::ExistentialDeposit::get();
-		let caller = account("caller", u, SEED);
+		let caller = account("caller", 0, SEED);
 
 		// Give some multiple of the existential deposit + creation fee + transfer fee
-		let balance = existential_deposit.saturating_mul(e.into());
+		let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
 		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&caller, balance);
 
 		// Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account, and reap this user.
-		let recipient: T::AccountId = account("recipient", u, SEED);
+		let recipient: T::AccountId = account("recipient", 0, SEED);
 		let recipient_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(recipient.clone());
-		let transfer_amount = existential_deposit.saturating_mul((e - 1).into()) + 1.into();
-	}: _(RawOrigin::Signed(caller), recipient_lookup, transfer_amount)
+		let transfer_amount = existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1.into();
+	}: transfer(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount)
 	verify {
+		assert_eq!(Balances::<T>::free_balance(&caller), Zero::zero());
 		assert_eq!(Balances::<T>::free_balance(&recipient), transfer_amount);
 	}
 
 	// Benchmark `transfer` with the best possible condition:
 	// * Both accounts exist and will continue to exist.
+	#[extra]
 	transfer_best_case {
-		let u in ...;
-		let e in ...;
-
-		let caller = account("caller", u, SEED);
-		let recipient: T::AccountId = account("recipient", u, SEED);
+		let caller = account("caller", 0, SEED);
+		let recipient: T::AccountId = account("recipient", 0, SEED);
 		let recipient_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(recipient.clone());
 
 		// Give the sender account max funds for transfer (their account will never reasonably be killed).
@@ -76,52 +70,80 @@ benchmarks! {
 		// Give the recipient account existential deposit (thus their account already exists).
 		let existential_deposit = T::ExistentialDeposit::get();
 		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&recipient, existential_deposit);
-		let transfer_amount = existential_deposit.saturating_mul(e.into());
-	}: transfer(RawOrigin::Signed(caller), recipient_lookup, transfer_amount)
+		let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
+	}: transfer(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount)
+	verify {
+		assert!(!Balances::<T>::free_balance(&caller).is_zero());
+		assert!(!Balances::<T>::free_balance(&recipient).is_zero());
+	}
 
 	// Benchmark `transfer_keep_alive` with the worst possible condition:
 	// * The recipient account is created.
 	transfer_keep_alive {
-		let u in ...;
-		let e in ...;
-
-		let caller = account("caller", u, SEED);
-		let recipient = account("recipient", u, SEED);
-		let recipient_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(recipient);
+		let caller = account("caller", 0, SEED);
+		let recipient: T::AccountId = account("recipient", 0, SEED);
+		let recipient_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(recipient.clone());
 
 		// Give the sender account max funds, thus a transfer will not kill account.
 		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value());
 		let existential_deposit = T::ExistentialDeposit::get();
-		let transfer_amount = existential_deposit.saturating_mul(e.into());
-	}: _(RawOrigin::Signed(caller), recipient_lookup, transfer_amount)
+		let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
+	}: _(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount)
+	verify {
+		assert!(!Balances::<T>::free_balance(&caller).is_zero());
+		assert_eq!(Balances::<T>::free_balance(&recipient), transfer_amount);
+	}
 
 	// Benchmark `set_balance` coming from ROOT account. This always creates an account.
-	set_balance {
-		let u in ...;
-		let e in ...;
-
-		let user: T::AccountId = account("user", u, SEED);
+	set_balance_creating {
+		let user: T::AccountId = account("user", 0, SEED);
 		let user_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(user.clone());
 
 		// Give the user some initial balance.
 		let existential_deposit = T::ExistentialDeposit::get();
-		let balance_amount = existential_deposit.saturating_mul(e.into());
+		let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
 		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&user, balance_amount);
-	}: _(RawOrigin::Root, user_lookup, balance_amount, balance_amount)
+	}: set_balance(RawOrigin::Root, user_lookup, balance_amount, balance_amount)
+	verify {
+		assert_eq!(Balances::<T>::free_balance(&user), balance_amount);
+		assert_eq!(Balances::<T>::reserved_balance(&user), balance_amount);
+	}
 
 	// Benchmark `set_balance` coming from ROOT account. This always kills an account.
 	set_balance_killing {
-		let u in ...;
-		let e in ...;
-
-		let user: T::AccountId = account("user", u, SEED);
+		let user: T::AccountId = account("user", 0, SEED);
 		let user_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(user.clone());
 
 		// Give the user some initial balance.
 		let existential_deposit = T::ExistentialDeposit::get();
-		let balance_amount = existential_deposit.saturating_mul(e.into());
+		let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
 		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&user, balance_amount);
-	}: set_balance(RawOrigin::Root, user_lookup, 0.into(), 0.into())
+	}: set_balance(RawOrigin::Root, user_lookup, Zero::zero(), Zero::zero())
+	verify {
+		assert!(Balances::<T>::free_balance(&user).is_zero());
+	}
+
+	// Benchmark `force_transfer` extrinsic with the worst possible conditions:
+	// * Transfer will kill the sender account.
+	// * Transfer will create the recipient account.
+	force_transfer {
+		let existential_deposit = T::ExistentialDeposit::get();
+		let source: T::AccountId = account("source", 0, SEED);
+		let source_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(source.clone());
+
+		// Give some multiple of the existential deposit + creation fee + transfer fee
+		let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
+		let _ = <Balances<T> as Currency<_>>::make_free_balance_be(&source, balance);
+
+		// Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account, and reap this user.
+		let recipient: T::AccountId = account("recipient", 0, SEED);
+		let recipient_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(recipient.clone());
+		let transfer_amount = existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1.into();
+	}: force_transfer(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount)
+	verify {
+		assert_eq!(Balances::<T>::free_balance(&source), Zero::zero());
+		assert_eq!(Balances::<T>::free_balance(&recipient), transfer_amount);
+	}
 }
 
 #[cfg(test)]
@@ -152,9 +174,9 @@ mod tests {
 	}
 
 	#[test]
-	fn transfer_set_balance() {
+	fn transfer_set_balance_creating() {
 		ExtBuilder::default().build().execute_with(|| {
-			assert_ok!(test_benchmark_set_balance::<Test>());
+			assert_ok!(test_benchmark_set_balance_creating::<Test>());
 		});
 	}
 
@@ -164,4 +186,11 @@ mod tests {
 			assert_ok!(test_benchmark_set_balance_killing::<Test>());
 		});
 	}
+
+	#[test]
+	fn force_transfer() {
+		ExtBuilder::default().build().execute_with(|| {
+			assert_ok!(test_benchmark_force_transfer::<Test>());
+		});
+	}
 }

substrate/frame/balances/src/lib.rs

@@ -180,19 +180,19 @@ use frame_system::{self as system, ensure_signed, ensure_root};
 pub use self::imbalances::{PositiveImbalance, NegativeImbalance};
 
 pub trait WeightInfo {
-	fn transfer(u: u32, e: u32, ) -> Weight;
-	fn transfer_best_case(u: u32, e: u32, ) -> Weight;
-	fn transfer_keep_alive(u: u32, e: u32, ) -> Weight;
-	fn set_balance(u: u32, e: u32, ) -> Weight;
-	fn set_balance_killing(u: u32, e: u32, ) -> Weight;
+	fn transfer() -> Weight;
+	fn transfer_keep_alive() -> Weight;
+	fn set_balance_creating() -> Weight;
+	fn set_balance_killing() -> Weight;
+	fn force_transfer() -> Weight;
 }
 
 impl WeightInfo for () {
-	fn transfer(_u: u32, _e: u32, ) -> Weight { 1_000_000_000 }
-	fn transfer_best_case(_u: u32, _e: u32, ) -> Weight { 1_000_000_000 }
-	fn transfer_keep_alive(_u: u32, _e: u32, ) -> Weight { 1_000_000_000 }
-	fn set_balance(_u: u32, _e: u32, ) -> Weight { 1_000_000_000 }
-	fn set_balance_killing(_u: u32, _e: u32, ) -> Weight { 1_000_000_000 }
+	fn transfer() -> Weight { 1_000_000_000 }
+	fn transfer_keep_alive() -> Weight { 1_000_000_000 }
+	fn set_balance_creating() -> Weight { 1_000_000_000 }
+	fn set_balance_killing() -> Weight { 1_000_000_000 }
+	fn force_transfer() -> Weight { 1_000_000_000 }
 }
 
 pub trait Subtrait<I: Instance = DefaultInstance>: frame_system::Trait {
@@ -462,7 +462,7 @@ decl_module! {
 		/// - DB Weight: 1 Read and 1 Write to destination account
 		/// - Origin account is already in memory, so no DB operations for them.
 		/// # </weight>
-		#[weight = T::DbWeight::get().reads_writes(1, 1) + 70_000_000]
+		#[weight = T::WeightInfo::transfer()]
 		pub fn transfer(
 			origin,
 			dest: <T::Lookup as StaticLookup>::Source,
@@ -491,7 +491,9 @@ decl_module! {
 		///     - Killing: 35.11 µs
 		/// - DB Weight: 1 Read, 1 Write to `who`
 		/// # </weight>
-		#[weight = T::DbWeight::get().reads_writes(1, 1) + 35_000_000]
+		#[weight = T::WeightInfo::set_balance_creating() // Creates a new account.
+			.max(T::WeightInfo::set_balance_killing()) // Kills an existing account.
+		]
 		fn set_balance(
 			origin,
 			who: <T::Lookup as StaticLookup>::Source,
@@ -533,7 +535,7 @@ decl_module! {
 		/// - Same as transfer, but additional read and write because the source account is
 		///   not assumed to be in the overlay.
 		/// # </weight>
-		#[weight = T::DbWeight::get().reads_writes(2, 2) + 70_000_000]
+		#[weight = T::WeightInfo::force_transfer()]
 		pub fn force_transfer(
 			origin,
 			source: <T::Lookup as StaticLookup>::Source,
@@ -557,7 +559,7 @@ decl_module! {
 		/// - Base Weight: 51.4 µs
 		/// - DB Weight: 1 Read and 1 Write to dest (sender is in overlay already)
 		/// #</weight>
-		#[weight = T::DbWeight::get().reads_writes(1, 1) + 50_000_000]
+		#[weight = T::WeightInfo::transfer_keep_alive()]
 		pub fn transfer_keep_alive(
 			origin,
 			dest: <T::Lookup as StaticLookup>::Source,

substrate/frame/benchmarking/src/lib.rs

@@ -188,6 +188,7 @@ macro_rules! benchmarks {
 			{ $( $( $where_ty: $where_bound ),* )? }
 			{ $( { $common , $common_from , $common_to , $common_instancer } )* }
 			( )
+			( )
 			$( $rest )*
 		);
 	}
@@ -210,6 +211,7 @@ macro_rules! benchmarks_instance {
 			{ $( $( $where_ty: $where_bound ),* )? }
 			{ $( { $common , $common_from , $common_to , $common_instancer } )* }
 			( )
+			( )
 			$( $rest )*
 		);
 	}
@@ -218,12 +220,34 @@ macro_rules! benchmarks_instance {
 #[macro_export]
 #[doc(hidden)]
 macro_rules! benchmarks_iter {
+	// detect and extract extra tag:
+	(
+		{ $( $instance:ident )? }
+		{ $( $where_clause:tt )* }
+		{ $( $common:tt )* }
+		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
+		#[extra]
+		$name:ident
+		$( $rest:tt )*
+	) => {
+		$crate::benchmarks_iter! {
+			{ $( $instance)? }
+			{ $( $where_clause )* }
+			{ $( $common )* }
+			( $( $names )* )
+			( $( $names_extra )* $name )
+			$name
+			$( $rest )*
+		}
+	};
 	// mutation arm:
 	(
 		{ $( $instance:ident )? }
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* ) // This contains $( $( { $instance } )? $name:ident )*
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* )
 		verify $postcode:block
 		$( $rest:tt )*
@@ -233,6 +257,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* )
+			( $( $names_extra )* )
 			$name { $( $code )* }: $name ( $origin $( , $arg )* )
 			verify $postcode
 			$( $rest )*
@@ -244,6 +269,7 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* )
 		verify $postcode:block
 		$( $rest:tt )*
@@ -253,6 +279,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* )
+			( $( $names_extra )* )
 			$name { $( $code )* }: {
 				<
 					Call<T $(, $instance)? > as $crate::frame_support::traits::UnfilteredDispatchable
@@ -270,6 +297,7 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: $eval:block
 		verify $postcode:block
 		$( $rest:tt )*
@@ -297,6 +325,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* { $( $instance )? } $name )
+			( $( $names_extra )* )
 			$( $rest )*
 		);
 	};
@@ -306,9 +335,19 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 	) => {
-		$crate::selected_benchmark!( { $( $where_clause)* } { $( $instance)? } $( $names )* );
-		$crate::impl_benchmark!( { $( $where_clause )* } { $( $instance)? } $( $names )* );
+		$crate::selected_benchmark!(
+			{ $( $where_clause)* }
+			{ $( $instance)? }
+			$( $names )*
+		);
+		$crate::impl_benchmark!(
+			{ $( $where_clause )* }
+			{ $( $instance)? }
+			( $( $names )* )
+			( $( $names_extra ),* )
+		);
 	};
 	// add verify block to _() format
 	(
@@ -316,6 +355,7 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* )
 		$( $rest:tt )*
 	) => {
@@ -324,6 +364,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* )
+			( $( $names_extra )* )
 			$name { $( $code )* }: _ ( $origin $( , $arg )* )
 			verify { }
 			$( $rest )*
@@ -335,6 +376,7 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* )
 		$( $rest:tt )*
 	) => {
@@ -343,6 +385,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* )
+			( $( $names_extra )* )
 			$name { $( $code )* }: $dispatch ( $origin $( , $arg )* )
 			verify { }
 			$( $rest )*
@@ -354,6 +397,7 @@ macro_rules! benchmarks_iter {
 		{ $( $where_clause:tt )* }
 		{ $( $common:tt )* }
 		( $( $names:tt )* )
+		( $( $names_extra:tt )* )
 		$name:ident { $( $code:tt )* }: $eval:block
 		$( $rest:tt )*
 	) => {
@@ -362,6 +406,7 @@ macro_rules! benchmarks_iter {
 			{ $( $where_clause )* }
 			{ $( $common )* }
 			( $( $names )* )
+			( $( $names_extra )* )
 			$name { $( $code )* }: $eval
 			verify { }
 			$( $rest )*
@@ -659,14 +704,20 @@ macro_rules! impl_benchmark {
 	(
 		{ $( $where_clause:tt )* }
 		{ $( $instance:ident )? }
-		$( { $( $name_inst:ident )? } $name:ident )*
+		( $( { $( $name_inst:ident )? } $name:ident )* )
+		( $( $name_extra:ident ),* )
 	) => {
 		impl<T: Trait $(<$instance>, I: Instance)? >
 			$crate::Benchmarking<$crate::BenchmarkResults> for Module<T $(, $instance)? >
 			where T: frame_system::Trait, $( $where_clause )*
 		{
-			fn benchmarks() -> Vec<&'static [u8]> {
-				vec![ $( stringify!($name).as_ref() ),* ]
+			fn benchmarks(extra: bool) -> Vec<&'static [u8]> {
+				let mut all = vec![ $( stringify!($name).as_ref() ),* ];
+				if !extra {
+					let extra = [ $( stringify!($name_extra).as_ref() ),* ];
+					all.retain(|x| !extra.contains(x));
+				}
+				all
 			}
 
 			fn run_benchmark(
@@ -931,10 +982,10 @@ macro_rules! impl_benchmark_test {
 macro_rules! add_benchmark {
 	( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => (
 		let name_string = stringify!($name).as_bytes();
-		let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist) = $params;
+		let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist, extra) = $params;
 		if &pallet[..] == &name_string[..] || &pallet[..] == &b"*"[..] {
 			if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] {
-				for benchmark in $( $location )*::benchmarks().into_iter() {
+				for benchmark in $( $location )*::benchmarks(extra).into_iter() {
 					$batches.push($crate::BenchmarkBatch {
 						results: $( $location )*::run_benchmark(
 							benchmark,

substrate/frame/benchmarking/src/utils.rs

@@ -66,6 +66,7 @@ sp_api::decl_runtime_apis! {
 			highest_range_values: Vec<u32>,
 			steps: Vec<u32>,
 			repeat: u32,
+			extra: bool,
 		) -> Result<Vec<BenchmarkBatch>, RuntimeString>;
 	}
 }
@@ -112,7 +113,11 @@ pub trait Benchmarking {
 pub trait Benchmarking<T> {
 	/// Get the benchmarks available for this pallet. Generally there is one benchmark per
 	/// extrinsic, so these are sometimes just called "extrinsics".
-	fn benchmarks() -> Vec<&'static [u8]>;
+	///
+	/// Parameters
+	/// - `extra`: Also return benchmarks marked "extra" which would otherwise not be
+	///            needed for weight calculation.
+	fn benchmarks(extra: bool) -> Vec<&'static [u8]>;
 
 	/// Run the benchmarks for this pallet.
 	///