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>

bin/node/executor/tests/basic.rs

@@ -312,6 +312,9 @@ fn full_native_block_import_works() {
 	let mut alice_last_known_balance: Balance = Default::default();
 	let mut fees = t.execute_with(|| transfer_fee(&xt()));
 
+	let transfer_weight = default_transfer_call().get_dispatch_info().weight;
+	let timestamp_weight = pallet_timestamp::Call::set::<Runtime>(Default::default()).get_dispatch_info().weight;
+
 	executor_call::<NeverNativeValue, fn() -> _>(
 		&mut t,
 		"Core_execute_block",
@@ -327,9 +330,8 @@ fn full_native_block_import_works() {
 		let events = vec![
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(0),
-				// timestamp set call with weight 8_000_000 + 2 read + 1 write
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 8_000_000 + 2 * 25_000_000 + 1 * 100_000_000, class: DispatchClass::Mandatory, ..Default::default() }
+					DispatchInfo { weight: timestamp_weight, class: DispatchClass::Mandatory, ..Default::default() }
 				)),
 				topics: vec![],
 			},
@@ -349,9 +351,8 @@ fn full_native_block_import_works() {
 			},
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(1),
-				// Balance Transfer 70_000_000 + 1 Read + 1 Write
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 70_000_000 + 25_000_000 + 100_000_000, ..Default::default() }
+					DispatchInfo { weight: transfer_weight, ..Default::default() }
 				)),
 				topics: vec![],
 			},
@@ -381,9 +382,8 @@ fn full_native_block_import_works() {
 		let events = vec![
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(0),
-				// timestamp set call with weight 8_000_000 + 2 read + 1 write
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 8_000_000 + 2 * 25_000_000 + 1 * 100_000_000, class: DispatchClass::Mandatory, ..Default::default() }
+					DispatchInfo { weight: timestamp_weight, class: DispatchClass::Mandatory, ..Default::default() }
 				)),
 				topics: vec![],
 			},
@@ -405,9 +405,8 @@ fn full_native_block_import_works() {
 			},
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(1),
-				// Balance Transfer 70_000_000 + 1 Read + 1 Write
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 70_000_000 + 25_000_000 + 100_000_000, ..Default::default() }
+					DispatchInfo { weight: transfer_weight, ..Default::default() }
 				)),
 				topics: vec![],
 			},
@@ -429,9 +428,8 @@ fn full_native_block_import_works() {
 			},
 			EventRecord {
 				phase: Phase::ApplyExtrinsic(2),
-				// Balance Transfer 70_000_000 + 1 Read + 1 Write
 				event: Event::frame_system(frame_system::RawEvent::ExtrinsicSuccess(
-					DispatchInfo { weight: 70_000_000 + 25_000_000 + 100_000_000, ..Default::default() }
+					DispatchInfo { weight: transfer_weight, ..Default::default() }
 				)),
 				topics: vec![],
 			},

bin/node/executor/tests/submit_transaction.rs

@@ -192,7 +192,7 @@ fn should_submit_signed_twice_from_all_accounts() {
 fn submitted_transaction_should_be_valid() {
 	use codec::Encode;
 	use frame_support::storage::StorageMap;
-	use sp_runtime::transaction_validity::{ValidTransaction, TransactionSource};
+	use sp_runtime::transaction_validity::{TransactionSource, TransactionTag};
 	use sp_runtime::traits::StaticLookup;
 
 	let mut t = new_test_ext(compact_code_unwrap(), false);
@@ -228,14 +228,12 @@ fn submitted_transaction_should_be_valid() {
 		<frame_system::Account<Runtime>>::insert(&address, account);
 
 		// check validity
-		let res = Executive::validate_transaction(source, extrinsic);
-
-		assert_eq!(res.unwrap(), ValidTransaction {
-			priority: 1_410_710_000_000,
-			requires: vec![],
-			provides: vec![(address, 0).encode()],
-			longevity: 2048,
-			propagate: true,
-		});
+		let res = Executive::validate_transaction(source, extrinsic).unwrap();
+
+		// We ignore res.priority since this number can change based on updates to weights and such.
+		assert_eq!(res.requires, Vec::<TransactionTag>::new());
+		assert_eq!(res.provides, vec![(address, 0).encode()]);
+		assert_eq!(res.longevity, 2048);
+		assert_eq!(res.propagate, true);
 	});
 }

bin/node/runtime/src/lib.rs

@@ -84,6 +84,9 @@ use impls::{CurrencyToVoteHandler, Author};
 pub mod constants;
 use constants::{time::*, currency::*};
 
+/// Weights for pallets used in the runtime.
+mod weights;
+
 // Make the WASM binary available.
 #[cfg(feature = "std")]
 include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));
@@ -322,7 +325,7 @@ impl pallet_balances::Trait for Runtime {
 	type Event = Event;
 	type ExistentialDeposit = ExistentialDeposit;
 	type AccountStore = frame_system::Module<Runtime>;
-	type WeightInfo = ();
+	type WeightInfo = weights::pallet_balances::WeightInfo;
 }
 
 parameter_types! {
@@ -1126,6 +1129,7 @@ impl_runtime_apis! {
 			highest_range_values: Vec<u32>,
 			steps: Vec<u32>,
 			repeat: u32,
+			extra: bool,
 		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
 			use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark};
 			// Trying to add benchmarks directly to the Session Pallet caused cyclic dependency issues.
@@ -1157,7 +1161,7 @@ impl_runtime_apis! {
 			];
 
 			let mut batches = Vec::<BenchmarkBatch>::new();
-			let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist);
+			let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist, extra);
 
 			add_benchmark!(params, batches, pallet_babe, Babe);
 			add_benchmark!(params, batches, pallet_balances, Balances);

bin/node/runtime/src/weights/mod.rs

@@ -0,0 +1,18 @@
+// Copyright (C) 2020 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! A list of the different weight modules for our runtime.
+
+pub mod pallet_balances;

bin/node/runtime/src/weights/pallet_balances.rs

@@ -0,0 +1,47 @@
+// Copyright (C) 2020 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Weights for the Balances Pallet
+
+use frame_support::weights::{Weight, constants::RocksDbWeight as DbWeight};
+
+pub struct WeightInfo;
+impl pallet_balances::WeightInfo for WeightInfo {
+	fn transfer() -> Weight {
+		(65949000 as Weight)
+			.saturating_add(DbWeight::get().reads(1 as Weight))
+			.saturating_add(DbWeight::get().writes(1 as Weight))
+	}
+	fn transfer_keep_alive() -> Weight {
+		(46665000 as Weight)
+			.saturating_add(DbWeight::get().reads(1 as Weight))
+			.saturating_add(DbWeight::get().writes(1 as Weight))
+	}
+	fn set_balance_creating() -> Weight {
+		(27086000 as Weight)
+			.saturating_add(DbWeight::get().reads(1 as Weight))
+			.saturating_add(DbWeight::get().writes(1 as Weight))
+	}
+	fn set_balance_killing() -> Weight {
+		(33424000 as Weight)
+			.saturating_add(DbWeight::get().reads(1 as Weight))
+			.saturating_add(DbWeight::get().writes(1 as Weight))
+	}
+	fn force_transfer() -> Weight {
+		(65343000 as Weight)
+			.saturating_add(DbWeight::get().reads(2 as Weight))
+			.saturating_add(DbWeight::get().writes(2 as Weight))
+	}
+}

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>());
+		});
+	}
 }