Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop Balances pallet erroneously double incrementing and decrementing consumers #1976

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Stop Balances pallet erroneously double incrementing and decrementing consumers #1976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5d2efd
fix locks ops causing consumer to double inc/dec
liamaharon Oct 23, 2023
a43f30c
fix comments
liamaharon Oct 23, 2023
7bf8226
Merge branch 'master' into liam-fix-double-consumer-inc-dec
liamaharon Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 5 additions & 22 deletions substrate/frame/balances/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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() {
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
true => Locks::<T, I>::remove(who),
false => Locks::<T, I>::insert(who, bounded_locks),
}

if prev_frozen > after_frozen {
Expand Down
24 changes: 23 additions & 1 deletion substrate/frame/balances/src/tests/currency_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
Expand All @@ -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));
});
}
Expand All @@ -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));
});
}
Expand All @@ -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));
});
}
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
}