Fix Index reuse for zero ExistentialDeposit configured chains#1966
Fix Index reuse for zero ExistentialDeposit configured chains#1966mnaamani wants to merge 4 commits intoparitytech:masterfrom
Conversation
…nt, or reaped this will prevent incorrect account index reuse
|
It looks like @mnaamani signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
1 similar comment
|
It looks like @mnaamani signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
|
Should my PR make necessary changes to runtime spec version and update package version and dependencies or is that left to maintainers? |
srml/balances/src/lib.rs
Outdated
| UpdateBalanceOutcome::AccountKilled | ||
| } else { | ||
| if !<FreeBalance<T>>::exists(who) { | ||
| if !<FreeBalance<T>>::exists(who) && !<ReservedBalance<T>>::exists(who) { |
There was a problem hiding this comment.
This should probably call Self::is_dead_account
|
I think your changes does alter the logic in a way, that you would need to modify more of the module to make this correct. But I'm no expert for this module. |
|
Let's wait for @gavofyork. |
|
The easy way (not sure if it is the best way) is just supply a trait implementation of |
|
I'm still unclear on precisely which circumstances this PR is designed to change. |
| { | ||
| fn is_dead_account(who: &T::AccountId) -> bool { | ||
| Self::total_balance(who).is_zero() | ||
| !<FreeBalance<T>>::exists(who) && !<ReservedBalance<T>>::exists(who) |
There was a problem hiding this comment.
Under what conditions do you expect these not to be equivalent?
There was a problem hiding this comment.
When existential deposit >= 1 they will always be equivalent because if the balances become too low, the FreeBalance::remove() and ReservedBalance::remove() will be called to remove the account and calling total_balance(who) will return zero.
The condition where these two are not equivalent is when existential deposit is equal to zero, with the expectation that an account will never be considered closed/dead after it has received a transfer, even if its total_balance goes to zero at some later time.
|
I think this one will not be merged, so I will close it. |
In chains that choose to allow zero ExistentialDeposit, the definition of a dead account is no longer the same as when existential deposit is greater than zero. In other words an account with zero balance is not considered dead. This has an impact on when an account index is (incorrectly) reused.
In addition it really only makes sense for an account index to remain associated with the same account as long as the total balance is still positive. After an account is reaped it is reasonable to create a new account index.
This PR addresses these two points:
prevents incorrect account index reuse by changing implementation of the
IsDeadAccounttrait for the balances module.Only calling new_account() when both the freebalance and reservedbalance components of an account don't exist