[pallet-balances] backport #3865 (#4398)

Backporting #3865 to 1.7 crates release for the `pallet-balances`.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

prdoc/pr_3865.prdoc

@@ -0,0 +1,11 @@
+title: "Balances: add failsafe for consumer ref underflow"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Pallet balances now handles the case that historic accounts violate a invariant that they should have a consumer ref on `reserved > 0` balance.  
+      This disallows such accounts from reaping and should prevent TI from getting messed up even more.
+
+crates:
+  - name: pallet-balances
+    bump: patch

substrate/frame/balances/Cargo.toml

@@ -28,6 +28,7 @@ docify = "0.2.6"
 
 [dev-dependencies]
 pallet-transaction-payment = { path = "../transaction-payment" }
+frame-support = { path = "../support", features = ["experimental"] }
 sp-core = { path = "../../primitives/core" }
 sp-io = { path = "../../primitives/io" }
 paste = "1.0.12"

substrate/frame/balances/src/lib.rs

@@ -959,6 +959,13 @@ pub mod pallet {
 				if !did_consume && does_consume {
 					frame_system::Pallet::<T>::inc_consumers(who)?;
 				}
+				if does_consume && frame_system::Pallet::<T>::consumers(who) == 0 {
+					// NOTE: This is a failsafe and should not happen for normal accounts. A normal
+					// account should have gotten a consumer ref in `!did_consume && does_consume`
+					// at some point.
+					log::error!(target: LOG_TARGET, "Defensively bumping a consumer ref.");
+					frame_system::Pallet::<T>::inc_consumers(who)?;
+				}
 				if did_provide && !does_provide {
 					// This could reap the account so must go last.
 					frame_system::Pallet::<T>::dec_providers(who).map_err(|r| {

substrate/frame/balances/src/tests/general_tests.rs

@@ -0,0 +1,111 @@
+// This file is part of Substrate.
+
+// Copyright (C) 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.
+
+#![cfg(test)]
+
+use crate::{
+	system::AccountInfo,
+	tests::{ensure_ti_valid, Balances, ExtBuilder, System, Test, TestId, UseSystem},
+	AccountData, ExtraFlags, TotalIssuance,
+};
+use frame_support::{
+	assert_noop, assert_ok, hypothetically,
+	traits::{
+		fungible::{Mutate, MutateHold},
+		tokens::Precision,
+	},
+};
+use sp_runtime::DispatchError;
+
+/// There are some accounts that have one consumer ref too few. These accounts are at risk of losing
+/// their held (reserved) balance. They do not just lose it - it is also not accounted for in the
+/// Total Issuance. Here we test the case that the account does not reap in such a case, but gets
+/// one consumer ref for its reserved balance.
+#[test]
+fn regression_historic_acc_does_not_evaporate_reserve() {
+	ExtBuilder::default().build_and_execute_with(|| {
+		UseSystem::set(true);
+		let (alice, bob) = (0, 1);
+		// Alice is in a bad state with consumer == 0 && reserved > 0:
+		Balances::set_balance(&alice, 100);
+		TotalIssuance::<Test>::put(100);
+		ensure_ti_valid();
+
+		assert_ok!(Balances::hold(&TestId::Foo, &alice, 10));
+		// This is the issue of the account:
+		System::dec_consumers(&alice);
+
+		assert_eq!(
+			System::account(&alice),
+			AccountInfo {
+				data: AccountData {
+					free: 90,
+					reserved: 10,
+					frozen: 0,
+					flags: ExtraFlags(1u128 << 127),
+				},
+				nonce: 0,
+				consumers: 0, // should be 1 on a good acc
+				providers: 1,
+				sufficients: 0,
+			}
+		);
+
+		ensure_ti_valid();
+
+		// Reaping the account is prevented by the new logic:
+		assert_noop!(
+			Balances::transfer_allow_death(Some(alice).into(), bob, 90),
+			DispatchError::ConsumerRemaining
+		);
+		assert_noop!(
+			Balances::transfer_all(Some(alice).into(), bob, false),
+			DispatchError::ConsumerRemaining
+		);
+
+		// normal transfers still work:
+		hypothetically!({
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			// Alice got back her consumer ref:
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+		hypothetically!({
+			assert_ok!(Balances::transfer_all(Some(alice).into(), bob, true));
+			// Alice got back her consumer ref:
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+
+		// un-reserving all does not add a consumer ref:
+		hypothetically!({
+			assert_ok!(Balances::release(&TestId::Foo, &alice, 10, Precision::Exact));
+			assert_eq!(System::consumers(&alice), 0);
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			assert_eq!(System::consumers(&alice), 0);
+			ensure_ti_valid();
+		});
+		// un-reserving some does add a consumer ref:
+		hypothetically!({
+			assert_ok!(Balances::release(&TestId::Foo, &alice, 5, Precision::Exact));
+			assert_eq!(System::consumers(&alice), 1);
+			assert_ok!(Balances::transfer_keep_alive(Some(alice).into(), bob, 40));
+			assert_eq!(System::consumers(&alice), 1);
+			ensure_ti_valid();
+		});
+	});
+}

substrate/frame/balances/src/tests/mod.rs

@@ -19,7 +19,7 @@
 
 #![cfg(test)]
 
-use crate::{self as pallet_balances, AccountData, Config, CreditOf, Error, Pallet};
+use crate::{self as pallet_balances, AccountData, Config, CreditOf, Error, Pallet, TotalIssuance};
 use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
 	assert_err, assert_noop, assert_ok, assert_storage_noop, derive_impl,
@@ -47,6 +47,7 @@ mod currency_tests;
 mod dispatchable_tests;
 mod fungible_conformance_tests;
 mod fungible_tests;
+mod general_tests;
 mod reentrancy_tests;
 
 type Block = frame_system::mocking::MockBlock<Test>;
@@ -298,6 +299,23 @@ pub fn info_from_weight(w: Weight) -> DispatchInfo {
 	DispatchInfo { weight: w, ..Default::default() }
 }
 
+/// Check that the total-issuance matches the sum of all accounts' total balances.
+pub fn ensure_ti_valid() {
+	let mut sum = 0;
+
+	for acc in frame_system::Account::<Test>::iter_keys() {
+		if UseSystem::get() {
+			let data = frame_system::Pallet::<Test>::account(acc);
+			sum += data.data.total();
+		} else {
+			let data = crate::Account::<Test>::get(acc);
+			sum += data.total();
+		}
+	}
+
+	assert_eq!(TotalIssuance::<Test>::get(), sum, "Total Issuance wrong");
+}
+
 #[test]
 fn weights_sane() {
 	let info = crate::Call::<Test>::transfer_allow_death { dest: 10, value: 4 }.get_dispatch_info();

substrate/frame/balances/src/types.rs

@@ -111,7 +111,7 @@ pub struct AccountData<Balance> {
 const IS_NEW_LOGIC: u128 = 0x80000000_00000000_00000000_00000000u128;
 
 #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, MaxEncodedLen, TypeInfo)]
-pub struct ExtraFlags(u128);
+pub struct ExtraFlags(pub(crate) u128);
 impl Default for ExtraFlags {
 	fn default() -> Self {
 		Self(IS_NEW_LOGIC)