[Fungibles] Avoid dusting below-ED amounts when creating/releasing holds

[karolk91]

During Paseo migration there was one account that fallen into edge case scenario that caused incorrect migration of said account. Due to the order of operation on balances when processing holds it is possible to erase/dust amounts below ED.

As far as I have checked: there were no accounts falling into this edge case for Kusama migration and there are about 50 accounts on Polkadot that will fall into this issue

Consider scenario:

• Assume existential deposit == 10
• Account has free balance == 15 (amount above ED)
• Account now creates hold with amount of 10 (amount enough to cover ED) so the reserve balance == 10
• What should be the free balance of the account?

Without this change the end result is:

• Free balance == 0
• Reserved balance == 10

This fix results in:

• Free balance == 5
• Reserved balance == 10

[karolk91]

/cmd prdoc --audience runtime_dev --bump patch

[muharem]

some comments. I approve when they addressed

[muharem]

@gui1117 your review would be helpful here

[ggwpez]

Do these emit evenets? Then we have to doc that there is a change. I think indexers may rely on the event order,

[karolk91]

yes, as far as I can tell by looking at code, both increase_balance and decrease_balance_on_hold will end up calling

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

Line 1064 in 92118ec

pub(crate) fn try_mutate_account<R, E: From<DispatchError>>(

and there can be events emitted like

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

Line 1154 in 92118ec

Pallet::<T, I>::deposit_event(Event::DustLost { account: who.clone(), amount });

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

Line 1148 in 92118ec

Self::deposit_event(Event::Endowed {

considering this change, it will cause Event::DustLost to be not emitted in some cases that would previously have this event emitted - but its ok because its the truth, we are avoiding dust so no event about DustLost. So its not the case of reordering events, rather have them emitted or not

[muharem]

case. free: { value < ED }. hold 1: { value < ED }. hold 2: { value > ED }.
if we try to release the first hold, it will fail with CannotCreate

Polkadot account: 5HQreXdyFQzM2hsKGq4PwMArh22VANb4Xg9ngLZREkg6dJN6

[gui1117]

We can add this test it will panic on a debug_assert:

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

Lines 327 to 330 in bafe782

	debug_assert!(
	maybe_dust.is_none(),
	"Does not alter main balance; dust only happens when it is altered; qed"
	);

#[test]
fn todo() {
	ExtBuilder::default()
		.existential_deposit(10)
		.monied(false)
		.build_and_execute_with(|| {
			<Balances as fungible::Mutate<_>>::set_balance(&1, 100);
			System::inc_providers(&1);
			assert_eq!(System::providers(&1), 2);

			assert_ok!(Balances::hold(&TestId::Foo, &1, 95));
			assert_eq!(Balances::balance_on_hold(&TestId::Foo, &1), 95);

			assert_eq!(Balances::free_balance(&1), 5);
			assert_eq!(System::providers(&1), 1);

			assert_ok!(Balances::burn_held(&TestId::Foo, &1, 95, Exact, Polite));
			assert_eq!(Balances::free_balance(&1), 5);
			assert_eq!(System::providers(&1), 2);
		});
}

But I think the debug_assert is maybe wrong and we can remove it, although it was written in the original PR implementing fungibles.

The thing is: the consumer reference is defined by holds>0 or frozen>0 and doesn't look at free balance, and simply dust when the hold is burnt, and fail when the hold is released and final free balance is less than E.D.
Maybe actually having the consumer reference defined as holds>0 or frozen>0 or free balance < E.D. is better but that may require a difficult migration.

For reference this is the code:

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

Lines 1071 to 1081 in f4e5e33

	let is_new = maybe_account.is_none();
	let mut account = maybe_account.take().unwrap_or_default();
	let did_provide =
	account.free >= Self::ed() && Self::have_providers_or_no_zero_ed(who);
	let did_consume =
	!is_new && (!account.reserved.is_zero() || !account.frozen.is_zero());
	let result = f(&mut account, is_new)?;
	let does_provide = account.free >= Self::ed();
	let does_consume = !account.reserved.is_zero() || !account.frozen.is_zero();

I can't find a complete specification for the fungible model, one model for the current implementation can be:

• if there is another provider then E.D. is not untouchable
• having holds is a consumer reference
• if there is another provider and if there are holds then we can have free balance lower than E.D. on the side.
• if there is another provider, and a hold less than E.D. and when we release free balance is less then E.D. then it fails with CannotCreate (My proposal for another PR would be to consider an alternative and actually keep the consumer reference while holds>0 or frozen>0 or free balance < E.D. so we don't fail and we don't dust.)