Inconsistent behaviour of repatriate_reserved after introducing account reference provider when ED is 0

[liuchengxu]

Problem description

I found this when I was updating Substrate to bb0fb29 which includes the PR paritytech/substrate#7363 in ChainX chainx-org/ChainX@b097d6e . After fixing the compilation, there are still four tests that failed, I later found out they failed because of repatriate_reserved no longer works like before, see https://github.com/chainx-org/ChainX/blob/9394bef6f88521715a724ff6407f238e6698cf48/xpallets/dex/spot/src/execution/asset.rs#L116-L121, the details will be explained later.

The interesting thing is that when I replace repatriate_reserved() with unreserve() + transfer(), the tests pass, see chainx-org/ChainX@f12cfa6.

Reproduction

Then I try to reproduce this issue in Substrate.

Test case:

1. ED is 0.
2. transfer Bob's reserved to Alice, the balance of Alice is all 0 but has non-zero consumers and providers.

Alice: AccountInfo {
    nonce: 0,
    consumers: 1,
    providers: 1,
    data: AccountData {
        free: 0,
        reserved: 0,
        misc_frozen: 0,
        fee_frozen: 0,
    },
}

Bob: AccountInfo {
    nonce: 0,
    consumers: 0,
    providers: 1,
    data: AccountData {
        free: 1500,
        reserved: 500,
        misc_frozen: 0,
        fee_frozen: 0,
    },
}

This is accepted before 7363, but reports DeadAccount error now.
Balances::repatriate_reserved(&bob, &alice, 500, Status::Free) = Err(
    DispatchError::Module {
        index: 0,
        error: 7,
        message: Some(
            "DeadAccount",
        ),
    },
)

test.patch:

diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs
index 7cb9b9d50..fa2147e03 100644
--- a/frame/balances/src/tests_composite.rs
+++ b/frame/balances/src/tests_composite.rs
@@ -147,3 +147,36 @@ impl ExtBuilder {
 }

 decl_tests!{ Test, ExtBuilder, EXISTENTIAL_DEPOSIT }
+
+#[test]
+fn repatriate_reserved_works_for_zero_account() {
+    // These functions all use `mutate_account` which may introduce a storage change when
+    // the account never existed to begin with, and shouldn't exist in the end.
+    <ExtBuilder>::default()
+        .existential_deposit(0)
+        .build()
+        .execute_with(|| {
+            let alice = 666;
+            let bob = 777;
+            assert_ok!(Balances::set_balance(Origin::root(), alice, 2000, 0));
+
+            assert_ok!(Balances::force_transfer(Origin::root(), alice, bob, 2000));
+
+            dbg!(666, frame_system::Account::<Test>::get(alice));
+            dbg!(777, frame_system::Account::<Test>::get(bob));
+
+            frame_system::Pallet::<Test>::inc_ref(&alice);
+            assert_ok!(Balances::reserve(&bob, 500));
+
+            dbg!(alice, frame_system::Account::<Test>::get(alice));
+            dbg!(bob, frame_system::Account::<Test>::get(bob));
+
+            // Repatriate Reserve
+            dbg!(Balances::repatriate_reserved(
+                &bob,
+                &alice,
+                500,
+                Status::Free
+            ));
+        });
+}

$ cd frame/balances/src

# Checkout the last commit before PR 7363
$ git checkout cfb8157f0
$ git apply test.patch
# test passes
$ cargo test zero_account -- --nocapture

# Checkout the commit with PR 7363
$ git checkout 7a79f54a5
$ git apply test.patch
# test failed
$ cargo test zero_account -- --nocapture

The beneficiary account Alice(zero balances with non-zero providers and consumers) is somehow None here
https://github.com/paritytech/substrate/blob/d0723f186662fd69ee37601367f51095d132df92/frame/balances/src/lib.rs#L655

[gui1117]

since paritytech/substrate#7363 you should use inc_consumers instead of inc_ref.
In your test inc_consumers is failing because there is no provider. If you want to make alice existing (i.e. provided) you need to do inc_providers instead.

Thus repatriate_reserved fail because beneficiary is not existing.
I guess the doc of repatriate_reserved should be explicit that beneficiary must be an existing account. Apart from that I don't see any issue here. (paritytech/substrate#8135)

Do I miss something ? Feel free to re-open if it doesn't solve your issue

EDIT: I missed the point, issue is re-opened

[liuchengxu]

@thiolliere Thanks for looking into this, but that does not solve this issue at all. Please help reopen it as I can't.

since paritytech/substrate#7363 you should use inc_consumers instead of inc_ref.

Yeah, I know inc_ref() is deprecated, but it merely calls inc_consumers() now, so the logic does not change and I just reused it here to make the reproduction steps easier.

Thus repatriate_reserved fail because beneficiary is not existing.

I'm guessing you don't run the test actually, Alice does exist but with all zero balances, consumers and providers are not zero, I think the non-zero consumers and providers indicates Alice is an existing account, right?

[frame/balances/src/tests_composite.rs:168] alice = 666
[frame/balances/src/tests_composite.rs:168] frame_system::Account::<Test>::get(alice) = AccountInfo {
    nonce: 0,
    consumers: 1,
    providers: 1,
    data: AccountData {
        free: 0,
        reserved: 0,
        misc_frozen: 0,
        fee_frozen: 0,
    },
}

And again, this must be a breaking change of this API since it behaves differently after the commit, making the project(ChainX) based on Substrate broken in this particular use case. This issue either needs to be really fixed to act like before or needs some explanation of why it behaves differently now.

Before paritytech/substrate#7363:

[frame/balances/src/tests_composite.rs:175] Balances::repatriate_reserved(&bob, &alice, 500, Status::Free) = Ok(
    0,
)

After paritytech/substrate#7363:

[frame/balances/src/tests_composite.rs:172] Balances::repatriate_reserved(&bob, &alice, 500, Status::Free) = Err(
    DispatchError::Module {
        index: 1,
        error: 7,
        message: Some(
            "DeadAccount",
        ),
    },
)

[gui1117]

sorry I read to fast.

Indeed the current implementation doesn't support existential deposit of 0 for balance using system account data. the StoredMap implementation for frame_system::Pallet says:

https://substrate.dev/rustdocs/v3.0.0/frame_system/pallet/struct.Pallet.html#impl-StoredMap%3C%3CT%20as%20Config%3E%3A%3AAccountId%2C%20%3CT%20as%20Config%3E%3A%3AAccountData%3E

Implement StoredMap for a simple single-item, provide-when-not-default system. This works fine for storing a single item which allows the account to continue existing as long as it's not empty/default.

Anything more complex will need more sophisticated logic.

(note that if you use don't use system account info but balance storage to store the balance then it works).

If you have existential deposit of 0 then every time an account gets back and forth to balance 0 the provider will be incremented.
so if we do: set alice balance to 500; transfer from alice to bob 500; transfer from bob to alice 500; then alice will have 2 providers.

also reading from the code, existing account from the point of view of pallet-balances means: account provided by pallet-balance.
If an account exist (like provided by another pallet) but is not provided by pallet-balance then we do deposit_into_existing it will fail.

To solve this issue I feel like there is 2 solutions:

• store Option<AccountData> in AccountInfo so that we can distinguish between balance 0 and not provided account.
  (seems better but it needs a storage migration for system AccountInfo)
• change StoredMap trait to make special case when existential deposit is 0. (but this seems difficult).

[liuchengxu]

Using Option<AccountData> sounds pretty straightforward.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[gui1117]

still relevant

[Mr-Leshiy]

I have tried literally modified test and with the latest master it passes.
Here it the PR where it has been reproduced paritytech/substrate#9463 .

[liuchengxu]

@Mr-Leshiy Thanks for trying that. You should add the test in tests_composite.rs as tests_composite.rs and tests_reentrancy.rs has a different test setup with regarding the AccountData, this issue is uncovered in test_reentrancy.rs.

[Mr-Leshiy]

@liuchengxu , Thank you I see, I have missed it.

[Mr-Leshiy]

I am not sure with the solution that I have provided, but what if we will just ask that account is exist and => it is consumed by others and this account has providers.
So the rule of the is_new here has been modified:
https://github.com/paritytech/substrate/pull/9463/files#diff-7f35e750129ddf17862e61be2ce506a120ac7d4de151633d2c026525130bba7dR903

At least such solution will not lead to using Option<AccountData> and migrating storage

[gui1117]

I got an idea. Instead of trying to find an implementation of StoredMap with AccountData being stored in system.
I think we should create a new implementation of StoredMap for when Option<AccountData> is being stored in system.
This way we should be able to implement it correctly with: provide when Some, non-provide when None.

Chain that have a ED of 0 will want to use this new data and will have to migrate their account in case they launched already. But existing chain with ED != 0 will not have to migrate and keep the current implementation.

I gave some snippet in this message paritytech/substrate#10117 (comment)

This seems to be the correct direction to pursue.

[liuchengxu]

Looks like a practical workaround, but can't say it's the ultimate solution as the framework has to maintain a guide somewhere for this edge case. When people wants to use 0 ED on their chain, they'd better learn about this history.

[gui1117]

I mean we will just 2 implementation of StoredMap in frame-system. One which stored the balance and cannot be used when ED is 0 the other which stored Option<balance> and can be used when ED is 0

[liuchengxu]

Hmm, does this mean it will be recommended to always use Option<AccountData> instead of plain AccountData when implementing frame_system::Config?