Use total balance (free + reserved) when performing liquidity checks for a new reserve (#8108)

# Description

Solves: #8099

Based on the documentation and existing code, the usable balance is
computed with the following formula:

```rs
// If Fortitude == Polite
let usable_balance = free - max(frozen - reserved, existential balance)
```

### The problem:

If an account's `free balance` is lower than `frozen balance`, no
reserves will be allowed even though the `usable balance` is enough to
cover the reserve, resulting in a `LiquidityRestrictions` error, which
should not happen.

### Visual example of how `usable/spendable` balance works:
```bash
|__total__________________________________|
|__on_hold__|_____________free____________|
|__________frozen___________|
|__on_hold__|__ed__|
            |__untouchable__|__spendable__|
```

## Integration

No action is required, the changes only change existing code, it does
not add or change any API.

## Review Notes

From my understanding, the function `ensure_can_withdraw` is incorrect,
and instead of checking that the new `free` balance is higher or equal
to the `frozen` balance, it should make sure the `new free` balance is
higher or equal to the `usable` balance.

---------

Co-authored-by: Kian Paimani <[email protected]>
(cherry picked from commit 7c2642d)

Author: 2 people

prdoc/pr_8108.prdoc

@@ -0,0 +1,16 @@
+title: "Fix `reserve` and `can_reserve` methods in `pallet-balances` to properly handle reserved balances"
+
+doc:
+- audience: Runtime Dev
+  description: |-
+    Fixes a bug in `pallet-balances` where the `reserve` and `can_reserve` methods were incorrectly 
+    preventing new reserves when an account's free balance was lower than its frozen balance, 
+    even if the usable balance was sufficient after accounting for the reserved balance.
+- audience: Runtime User
+  description: |-
+    Fixes a bug that was causing unexpected `LiquidityRestrictions` errors when attempting to 
+    reserve funds.
+
+crates: 
+- name: pallet-balances
+  bump: patch

substrate/frame/balances/src/impl_currency.rs

@@ -535,6 +535,29 @@ where
 	}
 }
 
+/// Validates whether an account can create a reserve without violating
+/// liquidity constraints.
+///
+/// This method performs liquidity checks without modifying the account state.
+fn ensure_can_reserve<T: Config<I>, I: 'static>(
+	who: &T::AccountId,
+	value: T::Balance,
+	check_existential_deposit: bool,
+) -> DispatchResult {
+	let AccountData { free, .. } = Pallet::<T, I>::account(who);
+
+	// Early validation: Check sufficient free balance
+	let new_free_balance = free.checked_sub(&value).ok_or(Error::<T, I>::InsufficientBalance)?;
+
+	// Conditionally validate existential deposit preservation
+	if check_existential_deposit {
+		let existential_deposit = T::ExistentialDeposit::get();
+		ensure!(new_free_balance >= existential_deposit, Error::<T, I>::Expendability);
+	}
+
+	Ok(())
+}
+
 impl<T: Config<I>, I: 'static> ReservableCurrency<T::AccountId> for Pallet<T, I>
 where
 	T::Balance: MaybeSerializeDeserialize + Debug,
@@ -546,11 +569,7 @@ where
 		if value.is_zero() {
 			return true
 		}
-		Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| {
-			new_balance >= T::ExistentialDeposit::get() &&
-				Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance)
-					.is_ok()
-		})
+		ensure_can_reserve::<T, I>(who, value, true).is_ok()
 	}
 
 	fn reserved_balance(who: &T::AccountId) -> Self::Balance {
@@ -570,7 +589,9 @@ where
 				account.free.checked_sub(&value).ok_or(Error::<T, I>::InsufficientBalance)?;
 			account.reserved =
 				account.reserved.checked_add(&value).ok_or(ArithmeticError::Overflow)?;
-			Self::ensure_can_withdraw(&who, value, WithdrawReasons::RESERVE, account.free)
+
+			// Check if it is possible to reserve before trying to mutate the account
+			ensure_can_reserve::<T, I>(who, value, false)
 		})?;
 
 		Self::deposit_event(Event::Reserved { who: who.clone(), amount: value });

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

@@ -258,7 +258,9 @@ fn lock_should_work_reserve() {
 				<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath),
 				TokenError::Frozen
 			);
-			assert_noop!(Balances::reserve(&1, 1), Error::<Test>::LiquidityRestrictions,);
+			// We can use frozen balance for reserves
+			assert_ok!(Balances::reserve(&1, 9));
+			assert_noop!(Balances::reserve(&1, 1), DispatchError::ConsumerRemaining,);
 			assert!(ChargeTransactionPayment::<Test>::validate_and_prepare(
 				ChargeTransactionPayment::from(1),
 				Some(1).into(),
@@ -280,6 +282,47 @@ fn lock_should_work_reserve() {
 		});
 }
 
+#[test]
+fn reserve_should_work_for_frozen_balance() {
+	ExtBuilder::default()
+		.existential_deposit(1)
+		.monied(true)
+		.build_and_execute_with(|| {
+			// Check balances
+			let account = Balances::account(&1);
+			assert_eq!(account.free, 10);
+			assert_eq!(account.frozen, 0);
+			assert_eq!(account.reserved, 0);
+
+			Balances::set_lock(ID_1, &1, 9, WithdrawReasons::RESERVE);
+
+			let account = Balances::account(&1);
+			assert_eq!(account.free, 10);
+			assert_eq!(account.frozen, 9);
+			assert_eq!(account.reserved, 0);
+
+			assert_ok!(Balances::reserve(&1, 5));
+
+			let account = Balances::account(&1);
+			assert_eq!(account.free, 5);
+			assert_eq!(account.frozen, 9);
+			assert_eq!(account.reserved, 5);
+
+			assert_ok!(Balances::reserve(&1, 4));
+
+			let account = Balances::account(&1);
+			assert_eq!(account.free, 1);
+			assert_eq!(account.frozen, 9);
+			assert_eq!(account.reserved, 9);
+
+			// Check usable balance
+			// usable_balance = free - max(frozen - reserved, ExistentialDeposit)
+			// 0 = 1 - max(9 - 9, 1)
+			let usable_balance = Balances::usable_balance(&1);
+			assert_eq!(usable_balance, 0);
+		});
+}
+
 #[test]
 fn lock_should_work_tx_fee() {
 	ExtBuilder::default()
@@ -291,7 +334,9 @@ fn lock_should_work_tx_fee() {
 				<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath),
 				TokenError::Frozen
 			);
-			assert_noop!(Balances::reserve(&1, 1), Error::<Test>::LiquidityRestrictions,);
+			// We can use frozen balance for reserves
+			assert_ok!(Balances::reserve(&1, 9));
+			assert_noop!(Balances::reserve(&1, 1), DispatchError::ConsumerRemaining,);
 			assert!(ChargeTransactionPayment::<Test>::validate_and_prepare(
 				ChargeTransactionPayment::from(1),
 				Some(1).into(),
@@ -1273,8 +1318,14 @@ fn reserve_must_succeed_if_can_reserve_does() {
 	ExtBuilder::default().build_and_execute_with(|| {
 		let _ = Balances::deposit_creating(&1, 1);
 		let _ = Balances::deposit_creating(&2, 2);
+		let _ = Balances::deposit_creating(&3, 3);
 		assert!(Balances::can_reserve(&1, 1) == Balances::reserve(&1, 1).is_ok());
 		assert!(Balances::can_reserve(&2, 1) == Balances::reserve(&2, 1).is_ok());
+
+		// Ensure that we can reserve as long (free + reserved) remains above
+		// the maximum of frozen balance.
+		Balances::set_lock(ID_1, &3, 2, WithdrawReasons::RESERVE);
+		assert_eq!(Balances::can_reserve(&3, 2), Balances::reserve(&3, 2).is_ok());
 	});
 }