From 0aeab381380107a6462fea240181f6e2a78f21b8 Mon Sep 17 00:00:00 2001 From: Liam Aharon 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 c6a2252df610..d518f933df8d 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::::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::::inc_consumers(who).defensive(); } if !did_consume && does_consume { @@ -1006,8 +1006,8 @@ pub mod pallet { let freezes = Freezes::::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::::contains_key(who); - if locks.is_empty() { - Locks::::remove(who); - if existed { - // TODO: use Locks::::hashed_key - // https://github.com/paritytech/substrate/issues/4969 - system::Pallet::::dec_consumers(who); - } - } else { - Locks::::insert(who, bounded_locks); - if !existed && system::Pallet::::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::::remove(who), + false => Locks::::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 2449638788dd..200df9ae743c 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!(>::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!(>::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!(>::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!(>::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!( >::transfer(&1, &2, 6, AllowDeath), TokenError::Frozen ); Balances::extend_lock(ID_1, &1, 2, WithdrawReasons::all()); + assert_eq!(System::consumers(&1), 1); assert_noop!( >::transfer(&1, &2, 6, AllowDeath), TokenError::Frozen ); Balances::extend_lock(ID_1, &1, 8, WithdrawReasons::all()); + assert_eq!(System::consumers(&1), 1); assert_noop!( >::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!(>::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!(>::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!(>::set_freeze(&TestId::Foo, &1, 0)); + assert_eq!(Balances::account(&1).frozen, 0); + assert_eq!(System::consumers(&1), 0); }); }