Substrate: Is `TotalIssuance` updated correctly?

[shawntabrizi]

Treasury module maintains extra funds in a Pot which is later distributed without changing the TotalIssuance by calling increase_free_balance_creating.

However, when funds are initially placed into the pot, TotalIssuance does not seem to increase, thus at some point, it seems like we have lost track of the correct total issued units.

Here are some relevant code blocks:
https://github.com/paritytech/substrate/blob/7af4c9c9cd6b689db5bcba5b76edc18bca80c781/srml/treasury/src/lib.rs#L244-L254

https://github.com/paritytech/substrate/blob/7af4c9c9cd6b689db5bcba5b76edc18bca80c781/srml/treasury/src/lib.rs#L199-L242

[shawntabrizi]

Another situation where TotalIssuance is not being handled well is in the reward() implementation.

If TotalIssuance were to ever overflow using increase_total_stake_by, it does not return an error, instead it just skips any logic:

https://github.com/paritytech/substrate/blob/7af4c9c9cd6b689db5bcba5b76edc18bca80c781/srml/balances/src/lib.rs#L358-L369

Then, in the reward() function, we increase the free balance of the user, without ever checking for the overflow of the TotalIssuance:

https://github.com/paritytech/substrate/blob/7af4c9c9cd6b689db5bcba5b76edc18bca80c781/srml/balances/src/lib.rs#L451-L458

[thiolliere]

on issue 1:

I thought funding assigned to Pot was coming from an amount created by staking and unassigned, but actually no this amount is created in on_dilution function. To solve it we can just reward beneficiary instead of increase_free_balance_creating. (also if we want to have the amount in the pot included in total_issuance then we should make a treasury account reward and slashed as such no ?)

If this is right then I also propose to remove increase_free_balance_creating which I introduced only for this purpose because I thought amount coming from on_dilution was unassigned but already included in total_issuance.

on issue 2:

how much do we rely on the fact that total issuance is equal to the real total issuance ? I mean for example here we double check:
https://github.com/paritytech/substrate/blob/7af4c9c9cd6b689db5bcba5b76edc18bca80c781/srml/balances/src/lib.rs#L307-L312

[shawntabrizi]

@thiolliere To follow up on issue 2

I agree we are a bit inconsistent in the Balances module when doing some math regarding an individuals balance. If we maintain that TotalIssuance is all possible units across accounts, then we should not need to check when adding to a user's balance for overflow since TotalIssuance should overflow well before any particular user. However, if it is better to be safe than sorry, there is quite a number of places we should update the code.

[joepetrowski]

Also a problem with balances/lib.rs:

fn set_balance(
    who: <T::Lookup as StaticLookup>::Source,
    #[compact] free: T::Balance,
    #[compact] reserved: T::Balance
) {
    let who = T::Lookup::lookup(who)?;
    Self::set_free_balance(&who, free);
    Self::set_reserved_balance(&who, reserved);
}

The functions set_X_balance do not check if the new balances are the same as the old balances.

[gavofyork]

If we maintain that TotalIssuance is all possible units across accounts

This is its definition, yes.

Therefore, yes, account balances may be added together without further checks.

[gavofyork]

The functions set_X_balance do not check if the new balances are the same as the old balances.

So what?

[gavofyork]

The TransferAsset::withdraw and ::deposit will need to be refactored to remove the alteration in issuance. We should add two new functions: mint and burn which do alter the issuance. Anywhere that we're currently withdrawing or otherwise removing balance from an account should be scrutinised and probably have a hook attached so that different chains can handle it in different ways.

[gavofyork]

https://github.com/paritytech/substrate/pull/2028 makes a start, but I'd like a complete audit of TotalIssuance, including all mint and burn points in the runtime (paying for gas, treasury pot, ...).

[joepetrowski]

The functions set_X_balance do not check if the new balances are the same as the old balances.

So what?

So if the new free_balance + reserved_balance is greater than or less than the old free_balance + reserved_balance, then it will be out of sync with TotalIssuance. I think we either need to have the check and update TotalIssuance accordingly, or add it to the docs that the caller needs to handle that.

[shawntabrizi]

My gut feel is that there should be no public functions which leaves it to the implementer to correctly manage total issuance. Additionally, I don't think management of total issuance should be public (as it is now).

Seems like we should have a function which allows an increase/decrease in either free balance or reserved balance of a user, and a function which transfers from reserved to free balance and vice versa. All of these should be able to manage the total issuance accurately and safely.

• increase_free_balance
• decrease_free_balance
• increase_reserved_balance
• decrease_reserved_balance
• reserve
• unreserve

Is there a reason to have any other exposed functions?

EDIT: I think the functions above make sense for the pure balances module, and then in the exposed Currency trait you can have an additional layer of funcitons:

• Slash (decrease free balance, dipping into reserved balance)
• Transfer
• Creation/Deletion of accounts with new balance amount
• etc....

[thiolliere]

note:

• reserve, unreserve are in Currency traits,
• decrease_free_balance is slash in Currency trait
  EDIT: false: slash also take from reserved_balances if not enough in free_balances (documentation state it but I expected not, documentation could emphase on it a bit more imho)
• increase_free_balance is reward in Currency trait (though it needs the account to pre-exixt)
• increase_reserved_balance and decrease_reserved_balance can already be done with combinations of above.

[thiolliere]

in our codebase the only modules that uses set_free_balance, set_free_balance_creating, increase_total_stake, increase_total_stake_by outside of tests is contract module.

however 2 usage can be replaced with reward and slash (for gas buy and refund)

Last usage is in accound_db and might be changed in two way:

• use of reward and slash
• or use a transfer without fees thing because actually the the overlay is only used as such it seems:
  https://github.com/paritytech/substrate/blob/91fa6594fe734ae41a2019cbc277cd175e7e91b9/srml/contract/src/exec.rs#L530-L535

But in both cases it may alter logic a bit as an account can be killed by a slash even cannot be rewarded after because it doesn't pre-exist anymore.

it seems a reward_creating method in currency would be needed then

[shawntabrizi]

decrease_free_balance is slash in Currency trait
increase_free_balance is reward in Currency trait

Is it crazy to want to rename these functions to be more clear? For example, I could imagine that slash is a function which not only removed from free balance, but also takes from reserved balance if not enough free balance exists. (Slash also seems like a punishment word, rather than some accounting)

is contract module.

@pepyakin

[joepetrowski]

For example, I could imagine that slash is a function which not only removed from free balance, but also takes from reserved balance if not enough free balance exists.

Slash already does this

    fn slash(who: &T::AccountId, value: Self::Balance) -> Option<Self::Balance> {
        let free_balance = Self::free_balance(who);
        let free_slash = cmp::min(free_balance, value);
        Self::set_free_balance(who, free_balance - free_slash);
        Self::decrease_total_stake_by(free_slash);
        if free_slash < value {
            Self::slash_reserved(who, value - free_slash)
        } else {
            None
        }
    }

[shawntabrizi]

Slash already does this

I see, then yes it does not exactly do what I am suggesting in my original post. I think there should be a more bare bones implementation of decrease_free_balance which can at most reduce to 0 the free balance.

[pepyakin]

The idea is that exec::transfer does exactly the same what balances::make_transfer would do but within the AccountDb environment (i.e. an entire scope of changes/transfers should be able to be reverted, and all changes are written to the storage once committed). It seems to me that set_free_balance_creating is a very appropriate function here because it just dumps the precomputed state (i.e. exact value of balances) into the storage.

Because they are all transfers between contracts and accounts you can assume that the total issuance will not be greater than it was before transfers. (I say "not greater" because it is possible that if a contract sends all their funds, the account will be deleted which can lead to leftovers of existential deposit to be burned)

or use a transfer without fees thing because actually the the overlay is only used as such it seems:

I'm not sure how one would use transfer since we we don't retain information about transfers in AccountDb, only the final balances after transfers were performed. We could record all transfers alongside the resulting balances and replay the transfers at the commit stage. The fact that this stores more info with each transfer worries me.

use of reward and slash

In principle, we can "reward" an account if we ended up with a balance higher than the current or "slash" if lower. But yeah, we should watch out for subtleties such as you've mentioned, @thiolliere.

[gavofyork]

https://github.com/paritytech/substrate/pull/2028 makes a step toward a clearer API for this. i would like to hide increase_total_stake_by and decrease_total_stake_by behind a type barrier so they can only be used during a OnUnbalancedDecrease/Increase; if i can come up with a clean way of doing it, then i'll do it tomorrow.

[thiolliere]

what During a OnUnbalancedDecrase means ? the associated type is constant so either it is a runtime check or you meant something like "inside".
I don't understand what does OnUnbalancedDecrease abstract ? could you instance an other implementation of it ?

[shawntabrizi]

I have made a "run and gun" version of the balances module as I think makes the most sense (to me):
https://github.com/paritytech/substrate/compare/gav-fixes...shawntabrizi:shawntabrizi-balances-v2

I have removed the concept of increase or decrease total issuance.

The only way to change free_balance or reserved_balance of a user is through {increase, decrease}_free_balance or {increase, decrease}_reserved_balance.

increase_x_balance handles account creation directly. In the situation where TotalIssuance would overflow, it does not make any changes to the state of the balances module.

decrease_x_balance handles account deletion directly. In the situation where a user's balance would underflow, it does not make any changes to the sate of the balances module.

set_x_balance simply checks if the new balance is less, greater, or equal to the old balance, and calls the appropriate decrease or increase function.

It seems with this set up, total_issuance cannot be messed up.

[rphmeier]

@shawntabrizi how does that work with e.g. minting into treasury?

[shawntabrizi]

@rphmeier my understanding is that ultimately funds in treasury make their way into a pot, and then from the pot to users. At the time of transferring from the pot to a user, you can call increase_free_balance and it will manage the total issuance automatically. To be honest, I am not 100% sure exactly the interactions of treasury, but I will look.

EDIT: I think we found in the old version of treasury, that the total issuance wasn't being tracked. Now that it is, it look pretty much exactly like increase_free_balance AFAIK

[gavofyork]

TotalIssuance is now strictly tracked by changes in #2048 . If there are follow-ups (e.g. to how treasury accounts), then it should be a separate issue.