diff --git a/prdoc/pr_3865.prdoc b/prdoc/pr_3865.prdoc new file mode 100644 index 000000000000..8e39c04825b1 --- /dev/null +++ b/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 diff --git a/substrate/frame/balances/Cargo.toml b/substrate/frame/balances/Cargo.toml index 6f4077237e7d..dc8ff08e035d 100644 --- a/substrate/frame/balances/Cargo.toml +++ b/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" diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs index 7dd087eabd63..eb71104f34b5 100644 --- a/substrate/frame/balances/src/lib.rs +++ b/substrate/frame/balances/src/lib.rs @@ -959,6 +959,13 @@ pub mod pallet { if !did_consume && does_consume { frame_system::Pallet::::inc_consumers(who)?; } + if does_consume && frame_system::Pallet::::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::::inc_consumers(who)?; + } if did_provide && !does_provide { // This could reap the account so must go last. frame_system::Pallet::::dec_providers(who).map_err(|r| { diff --git a/substrate/frame/balances/src/tests/general_tests.rs b/substrate/frame/balances/src/tests/general_tests.rs new file mode 100644 index 000000000000..0f3e015d0a89 --- /dev/null +++ b/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::::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(); + }); + }); +} diff --git a/substrate/frame/balances/src/tests/mod.rs b/substrate/frame/balances/src/tests/mod.rs index 91452b292b56..b123f01a15ae 100644 --- a/substrate/frame/balances/src/tests/mod.rs +++ b/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; @@ -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::::iter_keys() { + if UseSystem::get() { + let data = frame_system::Pallet::::account(acc); + sum += data.data.total(); + } else { + let data = crate::Account::::get(acc); + sum += data.total(); + } + } + + assert_eq!(TotalIssuance::::get(), sum, "Total Issuance wrong"); +} + #[test] fn weights_sane() { let info = crate::Call::::transfer_allow_death { dest: 10, value: 4 }.get_dispatch_info(); diff --git a/substrate/frame/balances/src/types.rs b/substrate/frame/balances/src/types.rs index 69d33bb023f3..3e36a83575c8 100644 --- a/substrate/frame/balances/src/types.rs +++ b/substrate/frame/balances/src/types.rs @@ -111,7 +111,7 @@ pub struct AccountData { 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)