Better Handle Dead Accounts in Balances (#7843)

* Don't mutate storage when account is dead and should stay dead

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_balances --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/balances/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* more concrete storage noop

Co-authored-by: Parity Benchmarking Bot <admin@parity.io>

Author: shawntabrizi

frame/balances/src/lib.rs

@@ -982,7 +982,7 @@ impl<T: Config<I>, I: Instance> Currency<T::AccountId> for Module<T, I> where
 	/// Slash a target account `who`, returning the negative imbalance created and any left over
 	/// amount that could not be slashed.
 	///
-	/// Is a no-op if `value` to be slashed is zero.
+	/// Is a no-op if `value` to be slashed is zero or the account does not exist.
 	///
 	/// NOTE: `slash()` prefers free balance, but assumes that reserve balance can be drawn
 	/// from in extreme circumstances. `can_slash()` should be used prior to `slash()` to avoid having
@@ -993,6 +993,7 @@ impl<T: Config<I>, I: Instance> Currency<T::AccountId> for Module<T, I> where
 		value: Self::Balance
 	) -> (Self::NegativeImbalance, Self::Balance) {
 		if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) }
+		if Self::is_dead_account(&who) { return (NegativeImbalance::zero(), value) }
 
 		Self::mutate_account(who, |account| {
 			let free_slash = cmp::min(account.free, value);
@@ -1146,9 +1147,10 @@ impl<T: Config<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I
 
 	/// Unreserve some funds, returning any amount that was unable to be unreserved.
 	///
-	/// Is a no-op if the value to be unreserved is zero.
+	/// Is a no-op if the value to be unreserved is zero or the account does not exist.
 	fn unreserve(who: &T::AccountId, value: Self::Balance) -> Self::Balance {
 		if value.is_zero() { return Zero::zero() }
+		if Self::is_dead_account(&who) { return value }
 
 		let actual = Self::mutate_account(who, |account| {
 			let actual = cmp::min(account.reserved, value);
@@ -1166,12 +1168,13 @@ impl<T: Config<I>, I: Instance> ReservableCurrency<T::AccountId> for Module<T, I
 	/// Slash from reserved balance, returning the negative imbalance created,
 	/// and any amount that was unable to be slashed.
 	///
-	/// Is a no-op if the value to be slashed is zero.
+	/// Is a no-op if the value to be slashed is zero or the account does not exist.
 	fn slash_reserved(
 		who: &T::AccountId,
 		value: Self::Balance
 	) -> (Self::NegativeImbalance, Self::Balance) {
 		if value.is_zero() { return (NegativeImbalance::zero(), Zero::zero()) }
+		if Self::is_dead_account(&who) { return (NegativeImbalance::zero(), value) }
 
 		Self::mutate_account(who, |account| {
 			// underflow should never happen, but it if does, there's nothing to be done here.

frame/balances/src/tests.rs

@@ -40,7 +40,7 @@ macro_rules! decl_tests {
 		use crate::*;
 		use sp_runtime::{FixedPointNumber, traits::{SignedExtension, BadOrigin}};
 		use frame_support::{
-			assert_noop, assert_ok, assert_err,
+			assert_noop, assert_storage_noop, assert_ok, assert_err,
 			traits::{
 				LockableCurrency, LockIdentifier, WithdrawReasons,
 				Currency, ReservableCurrency, ExistenceRequirement::AllowDeath, StoredMap
@@ -796,5 +796,28 @@ macro_rules! decl_tests {
 					);
 				});
 		}
+
+		#[test]
+		fn operations_on_dead_account_should_not_change_state() {
+			// These functions all use `mutate_account` which may introduce a storage change when
+			// the account never existed to begin with, and shouldn't exist in the end.
+			<$ext_builder>::default()
+				.existential_deposit(0)
+				.build()
+				.execute_with(|| {
+					assert!(!<Test as Config>::AccountStore::is_explicit(&1337));
+
+					// Unreserve
+					assert_storage_noop!(assert_eq!(Balances::unreserve(&1337, 42), 42));
+					// Reserve
+					assert_noop!(Balances::reserve(&1337, 42), Error::<Test, _>::InsufficientBalance);
+					// Slash Reserve
+					assert_storage_noop!(assert_eq!(Balances::slash_reserved(&1337, 42).1, 42));
+					// Repatriate Reserve
+					assert_noop!(Balances::repatriate_reserved(&1337, &1338, 42, Status::Free), Error::<Test, _>::DeadAccount);
+					// Slash
+					assert_storage_noop!(assert_eq!(Balances::slash(&1337, 42).1, 42));
+				});
+		}
 	}
 }

frame/balances/src/weights.rs

@@ -1,6 +1,6 @@
 // This file is part of Substrate.
 
-// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd.
+// Copyright (C) 2021 Parity Technologies (UK) Ltd.
 // SPDX-License-Identifier: Apache-2.0
 
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -15,9 +15,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//! Weights for pallet_balances
+//! Autogenerated weights for pallet_balances
+//!
 //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 2.0.0
-//! DATE: 2020-10-27, STEPS: [50, ], REPEAT: 20, LOW RANGE: [], HIGH RANGE: []
+//! DATE: 2021-01-06, STEPS: [50, ], REPEAT: 20, LOW RANGE: [], HIGH RANGE: []
 //! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled, CHAIN: Some("dev"), DB CACHE: 128
 
 // Executed Command:
@@ -48,76 +49,63 @@ pub trait WeightInfo {
 	fn set_balance_creating() -> Weight;
 	fn set_balance_killing() -> Weight;
 	fn force_transfer() -> Weight;
-	
 }
 
 /// Weights for pallet_balances using the Substrate node and recommended hardware.
 pub struct SubstrateWeight<T>(PhantomData<T>);
 impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	fn transfer() -> Weight {
-		(94_088_000 as Weight)
+		(100_698_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(1 as Weight))
 			.saturating_add(T::DbWeight::get().writes(1 as Weight))
-			
 	}
 	fn transfer_keep_alive() -> Weight {
-		(64_828_000 as Weight)
+		(69_407_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(1 as Weight))
 			.saturating_add(T::DbWeight::get().writes(1 as Weight))
-			
 	}
 	fn set_balance_creating() -> Weight {
-		(36_151_000 as Weight)
+		(38_489_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(1 as Weight))
 			.saturating_add(T::DbWeight::get().writes(1 as Weight))
-			
 	}
 	fn set_balance_killing() -> Weight {
-		(45_505_000 as Weight)
+		(48_458_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(1 as Weight))
 			.saturating_add(T::DbWeight::get().writes(1 as Weight))
-			
 	}
 	fn force_transfer() -> Weight {
-		(92_986_000 as Weight)
+		(99_320_000 as Weight)
 			.saturating_add(T::DbWeight::get().reads(2 as Weight))
 			.saturating_add(T::DbWeight::get().writes(2 as Weight))
-			
 	}
-	
 }
 
 // For backwards compatibility and tests
 impl WeightInfo for () {
 	fn transfer() -> Weight {
-		(94_088_000 as Weight)
+		(100_698_000 as Weight)
 			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
-			
 	}
 	fn transfer_keep_alive() -> Weight {
-		(64_828_000 as Weight)
+		(69_407_000 as Weight)
 			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
-			
 	}
 	fn set_balance_creating() -> Weight {
-		(36_151_000 as Weight)
+		(38_489_000 as Weight)
 			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
-			
 	}
 	fn set_balance_killing() -> Weight {
-		(45_505_000 as Weight)
+		(48_458_000 as Weight)
 			.saturating_add(RocksDbWeight::get().reads(1 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(1 as Weight))
-			
 	}
 	fn force_transfer() -> Weight {
-		(92_986_000 as Weight)
+		(99_320_000 as Weight)
 			.saturating_add(RocksDbWeight::get().reads(2 as Weight))
 			.saturating_add(RocksDbWeight::get().writes(2 as Weight))
-			
 	}
-	
 }

frame/support/src/lib.rs

@@ -499,6 +499,21 @@ macro_rules! assert_noop {
 	}
 }
 
+/// Evaluate any expression and assert that runtime storage has not been mutated
+/// (i.e. expression is a storage no-operation).
+///
+/// Used as `assert_storage_noop(expression_to_assert)`.
+#[macro_export]
+macro_rules! assert_storage_noop {
+	(
+		$x:expr
+	) => {
+		let h = $crate::storage_root();
+		$x;
+		assert_eq!(h, $crate::storage_root());
+	}
+}
+
 /// Assert an expression returns an error specified.
 ///
 /// Used as `assert_err!(expression_to_assert, expected_error_expression)`