From 0aeab381380107a6462fea240181f6e2a78f21b8 Mon Sep 17 00:00:00 2001
From: Liam Aharon <liam.aharon@hotmail.com>
Date: Mon, 30 Oct 2023 19:48:30 +1100
Subject: [PATCH] Stop `Balances` pallet erroneously double incrementing and
decrementing consumers (#1976)
Closes https://github.com/paritytech/polkadot-sdk/issues/1970
Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
https://github.com/paritytech/polkadot-sdk/issues/2037
---
substrate/frame/balances/src/lib.rs | 27 ++++---------------
.../balances/src/tests/currency_tests.rs | 24 ++++++++++++++++-
2 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/substrate/frame/balances/src/lib.rs b/substrate/frame/balances/src/lib.rs
index c6a2252df61..d518f933df8 100644
--- a/substrate/frame/balances/src/lib.rs
+++ b/substrate/frame/balances/src/lib.rs
@@ -935,8 +935,8 @@ pub mod pallet {
if did_provide && !does_provide {
// This could reap the account so must go last.
frame_system::Pallet::<T>::dec_providers(who).map_err(|r| {
+ // best-effort revert consumer change.
if did_consume && !does_consume {
- // best-effort revert consumer change.
let _ = frame_system::Pallet::<T>::inc_consumers(who).defensive();
}
if !did_consume && does_consume {
@@ -1006,8 +1006,8 @@ pub mod pallet {
let freezes = Freezes::<T, I>::get(who);
let mut prev_frozen = Zero::zero();
let mut after_frozen = Zero::zero();
- // TODO: Revisit this assumption. We no manipulate consumer/provider refs.
// No way this can fail since we do not alter the existential balances.
+ // TODO: Revisit this assumption.
let res = Self::mutate_account(who, |b| {
prev_frozen = b.frozen;
b.frozen = Zero::zero();
@@ -1024,26 +1024,9 @@ pub mod pallet {
debug_assert!(maybe_dust.is_none(), "Not altering main balance; qed");
}
- let existed = Locks::<T, I>::contains_key(who);
- if locks.is_empty() {
- Locks::<T, I>::remove(who);
- if existed {
- // TODO: use Locks::<T, I>::hashed_key
- // https://github.com/paritytech/substrate/issues/4969
- system::Pallet::<T>::dec_consumers(who);
- }
- } else {
- Locks::<T, I>::insert(who, bounded_locks);
- if !existed && system::Pallet::<T>::inc_consumers_without_limit(who).is_err() {
- // No providers for the locks. This is impossible under normal circumstances
- // since the funds that are under the lock will themselves be stored in the
- // account and therefore will need a reference.
- log::warn!(
- target: LOG_TARGET,
- "Warning: Attempt to introduce lock consumer reference, yet no providers. \
- This is unexpected but should be safe."
- );
- }
+ match locks.is_empty() {
+ true => Locks::<T, I>::remove(who),
+ false => Locks::<T, I>::insert(who, bounded_locks),
}
if prev_frozen > after_frozen {
diff --git a/substrate/frame/balances/src/tests/currency_tests.rs b/substrate/frame/balances/src/tests/currency_tests.rs
index 2449638788d..200df9ae743 100644
--- a/substrate/frame/balances/src/tests/currency_tests.rs
+++ b/substrate/frame/balances/src/tests/currency_tests.rs
@@ -144,7 +144,9 @@ fn lock_removal_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
Balances::remove_lock(ID_1, &1);
+ assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -156,7 +158,9 @@ fn lock_replacement_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -168,7 +172,9 @@ fn double_locking_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_2, &1, 5, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -179,8 +185,11 @@ fn combination_locking_should_work() {
.existential_deposit(1)
.monied(true)
.build_and_execute_with(|| {
+ assert_eq!(System::consumers(&1), 0);
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::empty());
+ assert_eq!(System::consumers(&1), 0);
Balances::set_lock(ID_2, &1, 0, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -192,16 +201,19 @@ fn lock_value_extension_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
TokenError::Frozen
);
Balances::extend_lock(ID_1, &1, 2, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
TokenError::Frozen
);
Balances::extend_lock(ID_1, &1, 8, WithdrawReasons::all());
+ assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 3, AllowDeath),
TokenError::Frozen
@@ -1324,9 +1336,14 @@ fn freezing_and_locking_should_work() {
.existential_deposit(1)
.monied(true)
.build_and_execute_with(|| {
+ // Consumer is shared between freezing and locking.
+ assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 4));
+ assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
- assert_eq!(System::consumers(&1), 2);
+ assert_eq!(System::consumers(&1), 1);
+
+ // Frozen and locked balances update correctly.
assert_eq!(Balances::account(&1).frozen, 5);
assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 6));
assert_eq!(Balances::account(&1).frozen, 6);
@@ -1336,8 +1353,13 @@ fn freezing_and_locking_should_work() {
assert_eq!(Balances::account(&1).frozen, 4);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(Balances::account(&1).frozen, 5);
+
+ // Locks update correctly.
Balances::remove_lock(ID_1, &1);
assert_eq!(Balances::account(&1).frozen, 4);
assert_eq!(System::consumers(&1), 1);
+ assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 0));
+ assert_eq!(Balances::account(&1).frozen, 0);
+ assert_eq!(System::consumers(&1), 0);
});
}
--
GitLab