Inconsistent Transferable Balance Calculation Logic

[ahmadkaouk]

An inconsistency has been identified in the Polkadot SDK concerning the logic used for computing transferable balances. This divergence may mislead users about the actual spendable balance, potentially leading to transaction failures due to inadequate fee coverage.

The inconsistency arises from the utilization of different formulas:

1. Old Formula: transferable = free - max(frozen, reserved).
2. New Formula: transferable = free - (frozen - reserved).

This originates from the CurrencyAdapter, which relies on the outdated Currency trait as illustrated here. The new transferable balance is computed utilizing the reducible_balances function defined here.

The discrepancy is particularly noticeable in the eth.getBalance method in Frontier, where the new formula is employed. Under certain circumstances, a positive balance may be displayed, indicating available funds. However, transactions may fail due to an insufficient balance to cover the fees, given that the old formula is utilized for fee calculations.

• Is the discrepancy in the logic for calculating "transferable balance" concerning fees a deliberate design or an oversight?
• Is there a rationale behind not extending the new logic to fee calculations, and if so, what is the reasoning behind this approach?

[crystalin]

This is partially fixed by #2038. At least the EVM balance side.

However, the fact the fees can be paid with untransferable balance still seem broken, is it because it requires more migration or something to fix @bkchr ?

[kianenigma]

IN any case, an item missing (and rather high priority) in #226 is then migrate CurrencyAdapter to also work with fungible, I added it there.

[michalisFr]

@ahmadkaouk Just to point that if the account cannot be reaped (which should be the case when there's a hold and/or freeze) or we don't want it to be reaped, the transferable balance calculation is this:
transferable = free - max(frozen - reserved, ED)
since now the ED is a requirement over the free balance, not the total balance.

[liamaharon]

Spendable balance indeed seems like it should be transferable = free - max(frozen, reserved), not transferable = free - (frozen - reserved) (@bkchr can you confirm?).

I will work on this.

[liamaharon]

@ahmadkaouk I have a couple of questions:

1. Could you please clarify why you determined this regression to have originated in the CurrencyAdaptor?

After initial review, it seems was originated by the change in behavior of fn reducible_balance introduced in paritytech/substrate#12951.

Prior implementation:

polkadot-sdk/substrate/frame/balances/src/lib.rs

Lines 1070 to 1083 in c699876

	fn reducible_balance(who: &T::AccountId, keep_alive: bool) -> Self::Balance {
	let a = Self::account(who);
	// Liquid balance is what is neither reserved nor locked/frozen.
	let liquid = a.free.saturating_sub(a.fee_frozen.max(a.misc_frozen));
	if frame_system::Pallet::<T>::can_dec_provider(who) && !keep_alive {
	liquid
	} else {
	// `must_remain_to_exist` is the part of liquid balance which must remain to keep total
	// over ED.
	let must_remain_to_exist =
	T::ExistentialDeposit::get().saturating_sub(a.total() - liquid);
	liquid.saturating_sub(must_remain_to_exist)
	}
	}

New implementation:

polkadot-sdk/substrate/frame/balances/src/impl_fungible.rs

Lines 44 to 70 in 495d24d

	fn reducible_balance(
	who: &T::AccountId,
	preservation: Preservation,
	force: Fortitude,
	) -> Self::Balance {
	let a = Self::account(who);
	let mut untouchable = Zero::zero();
	if force == Polite {
	// Frozen balance applies to total. Anything on hold therefore gets discounted from the
	// limit given by the freezes.
	untouchable = a.frozen.saturating_sub(a.reserved);
	}
	// If we want to keep our provider ref..
	if preservation == Preserve
	// ..or we don't want the account to die and our provider ref is needed for it to live..
	|| preservation == Protect && !a.free.is_zero() &&
	frame_system::Pallet::<T>::providers(who) == 1
	// ..or we don't care about the account dying but our provider ref is required..
	|| preservation == Expendable && !a.free.is_zero() &&
	!frame_system::Pallet::<T>::can_dec_provider(who)
	{
	// ..then the ED needed..
	untouchable = untouchable.max(T::ExistentialDeposit::get());
	}
	// Liquid balance is what is neither on hold nor frozen/required for provider.
	a.free.saturating_sub(untouchable)
	}

2. Where did you get this formula?

Old Formula: transferable = free - max(frozen, reserved).

It seems only frozen and free are used in the prior implementation, I don't see usage of reserved anywhere.

Thanks.

[gavofyork]

#12951 did alter the functionality of existing reserves by design:

Alter reserve functionality in Balances so that it does not contribute to the account's existential deposit and does use a consumer reference. Remove the concept of withdraw reasons from locks. Migration is lazy: accounts about to be mutated will be migrated prior. New accounts will automatically be mutated. A permissionless upgrade dispatchable is provided to upgrade dormant accounts.

That Frontier is now showing some inconsistencies in its getBalance (which presumably reports transferability rather than the real balance) indicates these repercussions have not yet been integrated. Introducing a test would probably help ensure this doesn't happen again.

[liamaharon]

@gavofyork I have a clarifying question about the Balances implementation of Inspect::reducible_balance.

Here's the doc comment describing the behavior of the method:

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/regular.rs

Lines 80 to 85 in c4211b6

	/// Get the maximum amount that `who` can withdraw/transfer successfully based on whether the
	/// account should be kept alive (`preservation`) or whether we are willing to force the
	/// reduction and potentially go below user-level restrictions on the minimum amount of the
	/// account.
	///
	/// Always less than or equal to `balance()`.

Here is the Balances implementation:

polkadot-sdk/substrate/frame/balances/src/impl_fungible.rs

Lines 44 to 70 in 495d24d

	fn reducible_balance(
	who: &T::AccountId,
	preservation: Preservation,
	force: Fortitude,
	) -> Self::Balance {
	let a = Self::account(who);
	let mut untouchable = Zero::zero();
	if force == Polite {
	// Frozen balance applies to total. Anything on hold therefore gets discounted from the
	// limit given by the freezes.
	untouchable = a.frozen.saturating_sub(a.reserved);
	}
	// If we want to keep our provider ref..
	if preservation == Preserve
	// ..or we don't want the account to die and our provider ref is needed for it to live..
	|| preservation == Protect && !a.free.is_zero() &&
	frame_system::Pallet::<T>::providers(who) == 1
	// ..or we don't care about the account dying but our provider ref is required..
	|| preservation == Expendable && !a.free.is_zero() &&
	!frame_system::Pallet::<T>::can_dec_provider(who)
	{
	// ..then the ED needed..
	untouchable = untouchable.max(T::ExistentialDeposit::get());
	}
	// Liquid balance is what is neither on hold nor frozen/required for provider.
	a.free.saturating_sub(untouchable)
	}

Is line 54 (untouchable = a.frozen.saturating_sub(a.reserved);) correct, or is it supposed to be untouchable = a.frozen.max(a.reserved);?

Otherwise, even when Inspect::reducible_balance is called with Polite it seems it can return a value greater than a.reserved and a.frozen, which doesn't seem right given the behavior described in the doc comment.

[gavofyork]

They're both correct.

If Polite, then we are unable to reduce the total balance to below the frozen balance. We work out what is "untouchable" from the free balance so that we understand how much is left which can be reduced. Since we can never touch the balance on hold anyway and since the balance on hold contributes to the total, then we can discount what is untouchable by this. This works because total == free + on_hold.

Perhaps the confusion stems from the fact that we consider frozen in relation to the total, but end up calculating reducible by subtracting what is untouchable from what is free.

[liamaharon]

Ah, thank you for clarifying.

Visual aid:

|_______total___________________|
|_______on_hold____|____free____|
|_______frozen___________|
                    xxxxx
                      ^
         untouchable = frozen - on_hold

[liamaharon]

@ahmadkaouk could you please confirm if updating the CurrencyAdaptor to use fungible (therefore the new formula) instead Currency should resolve this issue with Frontier?

Is there any test case I can use to reproduce this?

[gavofyork]

Visual aid:

Yes, and you can add the ED into that, too:

|__total__________________________________|
|__on_hold__|_____________free____________|
|__________frozen___________|
|__on_hold__|__ed__|        
            |__untouchable__|__spendable__|

untouchable = max(ed, frozen - on_hold)

so without anything frozen:

|__total__________________________________|
|__on_hold__|_____________free____________|
|__on_hold__|__ed__|______spendable_______|
            |......| <- untouchable

(or if we don't care about ED because we have other providers or will reap and don't have any consumers, then it's just untouchable = frozen - on_hold, as with your diagram above.)

[ahmadkaouk]

@ahmadkaouk could you please confirm if updating the CurrencyAdaptor to use fungible (therefore the new formula) instead Currency should resolve this issue with Frontier?

Is there any test case I can use to reproduce this?

@liamaharon, yes I believe that updating CurrencyAdapter to use fungible can solve the problem. I have created a test on the Moonbeam network that reproduces the issue here.

Here's a summary of the scenario that triggers the problem:

1. A new account is created with an initial balance of 30 GLMR.
2. The new account submits a proposal with a deposit of minDepositAmount (4 GLMR). (Total of 9 GLMR are reserved)
3. The new account delegates 20 GLMR.
4. Then the account transfers 5 GLMR to another account.
5. We verify that the account has a transferable balance of approximately 5 GLMR.
6. A second transfer of 2 GLMR is made from the new account.
7. The transfer fails with this error message "Invalid Transaction: Inability to pay some fees , e.g. account balance too low"

Balance info for the new account at the end:

  data: {
    free: 15,996,484,162,237,998,502
    reserved: 9,002,400,000,000,000,000
    frozen: 20,000,000,000,000,000,000
    flags: 170,141,183,460,469,231,731,687,303,715,884,105,728
  }

Note that ED is 0 in this scenario.

[liamaharon]

Hey @ahmadkaouk can try the FungibleAdapter from #2292?

[ggwpez]

We kind of need a unit test here in the SDK to prevent this from happening again and checking that #2292 does solve the issue.

[kianenigma]

Ah, thank you for clarifying.

Visual aid:

|_______total___________________|
|_______on_hold____|____free____|
|_______frozen___________|
                    xxxxx
                      ^
         untouchable = frozen - on_hold

What helped me understand this convo and digram(s) better is to specify that untouchable is the portion of free that is untouchable, not total. In other words I would call it free_untouchable.

[ahmadkaouk]

Hey @liamaharon, just wanted to let you know that I tested the FungibleAdapter from #2292 and it does fix the issue.

[liamaharon]

Hey @liamaharon, just wanted to let you know that I tested the FungibleAdapter from #2292 and it does fix the issue.

@ahmadkaouk fyi this is finally merged and will be included in the next release.

[bkchr]

As the fix is merged, we can close this?