pallet-staking: track total unstaking amount

[kianenigma]

In staking, we have a mapping as such: AccountId -> Ledger whereby Ledger { active, total }. What we need is a tracker over the sum(Ledger::active) and sum(Ledger::total) in the entire map. Subtracting the two should give the total unbonding amount.

As for the implementation, once #1654 is done, staking will have fully implement the OnStakingUpdate. This can be implemented as a component that is implementing OnStakingUpdate and using that.

I prefer an implementation that is actually just a stuct implementing the types, which can easily be embedded in a pallet. Then, we can decide to position this in the existing staking pallet or elsewhere.

I imagine we can use it to:

1. future fancier unstaking process.
2. implement things like preventing all of the DOTs to be unstaked at once etc.

[ruseinov]

Thinking about this, do you want to do this in a separate pallet or within the tracker? It could also fit into EventListener paradigm, but I gotta check the code first.

[kianenigma]

Having thought about it more, I am leaning toward it being in staking. But the final verdict can probably come after a bit of prototyping.

[ruseinov]

I was thinking about this. It's indeed not very useful to do this in an EventHandler, because all of this is going to happen in update_ledger only and it's a one-liner. The migration seems to be pretty straightforward in my head - just a multi-block migration that does not care about the changes in staking participants, because that would automatically update_ledger, which in turn would update the amount. So we just casually iterate all the keys while staking is doing it's thing, until we run out of keys. We could take up to half the block weight for this each block. Gotta see exactly how much weight this would consume for the current Ledger storage.

[kianenigma]

The migration seems to be pretty straightforward in my head - just a multi-block migration that does not care about the changes in staking participants, because that would automatically update_ledger, which in turn would update the amount.

This is easier said that done. Once I see us executing it successfully for something else, I would happily consider it as "just another migration".

[kianenigma]

About freezing the calls of a pallet, the system::BaseCallFilter might be an easier way, compared to just adding a manual if to dispatch functions.

But even if this works, the overhead of it is probably waaaay too much. Better just do it in the pallet.

[eagr]

I wanna help out if it’s ok. Trying to make sense of this.

The balance of an account that is being unstaked is stored on chain, but not the sum. We could either calculate it on the fly, or store the sum somewhere and update it on change of the unlocking queue. I guess it depends on when and how ofter the value is read, also how often the queue is updated. I don’t see the whole picture so I can’t tell. Either way, it seems like it should be done in StakingLedger for maintaining data integrity, right?

The hard part is a migration that sets the initial value.

Judging from this part (if the statement is still applicable), I assume you want to store the sum on chain. I imagine the tricky part is to somehow freeze the unlocking queue until after the initial value is calculated, right? Overall, the migration part is still vague to me.

I’m brand new to Substrate, so what’s obvious for you guys may not be so obvious for me. Care to elaborate a bit on these if it’s not too much trouble? :))

[liamaharon]

@Ank4n @gpestana maybe you can comment on this 👆

[Ank4n]

https://github.com/paritytech/polkadot-sdk/pull/1322

The balance of an account that is being unstaked is stored on chain, but not the sum. We could either calculate it on the fly, or store the sum somewhere and update it on change of the unlocking queue. I guess it depends on when and how ofter the value is read, also how often the queue is updated. I don’t see the whole picture so I can’t tell. Either way, it seems like it should be done in StakingLedger for maintaining data integrity, right?

We have individual ledgers and we want to store the global sum of unbonding stake as a separate storage item. From top of my head, we will need to modify this value everytime there is a new unbond (adding to it), a rebond or a withdraw (substracting from this). It cannot be done in StakingLedger but should be similar to how we added TotalValueLocked in this PR.

I imagine the tricky part is to somehow freeze the unlocking queue until after the initial value is calculated, right? Overall, the migration part is still vague to me.

The tricky part is, we need to initialise this value which does not exist right now. The calculation of this might not be possible in one block (since we need to iterate through all ledgers and its unlocking queue), and if we do across multiple blocks, we need to make sure the values of unbonding do not change while we are still calculating this (hence a need to freeze unbonding/withdraw or more simply all staking operations). The PR I linked above did a similar thing in one block and I would suggest to implement it similarly (single block migration). Once you have that, we could try benchmarking and see if it is feasible with one block or to try converting it into a multi-block. We also have a multi-block migration PR hopefully soon to be merged that can help with it.

Lmk if you have further questions.

[eagr]

There're two states after unbonding: being-unlocked and unlocked. From your explanation, I presume the new value should take account of both cases, correct? If so, would the name TotalValueUnlocked be good enough? As the name isn’t totally clear that it includes the amount that is being unlocked. @Ank4n

[Ank4n]

There're two states after unbonding: being-unlocked and unlocked. From your explanation, I presume the new value should take account of both cases, correct? If so, would the name TotalValueUnlocked be good enough? As the name isn’t totally clear that it includes the amount that is being unlocked. @Ank4n

Once its unlocked, it is free balance and does not need to be tracked (or in other words removed from our tracker when unlock happens). TotalValueUnlocked sounds incorrect as it implies tokens that has been unlocked and free. What we want to track is the token that is still bonded but not active and waiting to be unlocked. My recommendation would be to call it TotalValueUnbonding or just TotalUnbonding.

[eagr]

Sorry for the inaccurate terms. 😉

After unstaking some amount, it enters the unbonding period, during which we couldn't withdraw yet, let's call the total of this X. After the unbonding period, then we could withdraw, let's call the total of what haven't been withdrawn Y. I presume the new value should track X+Y, correct?

[gpestana]

The tricky part is, we need to initialise this value which does not exist right now.

I agree with @Ank4n that the tricky part in this task is to ensure the migrations are done correctly and if done in a multi-block fashion, the final values do not get out of sync.

After unstaking some amount, it enters the unbonding period, during which we couldn't withdraw yet, let's call the total of this X. After the unbonding period, then we could withdraw, let's call the total of what haven't been withdrawn Y. I presume the new value should track X+Y, correct?

This seems correct to me but probably it is unnecessarily complicated to separate the unlocking pre and post unbounding period. The goal is keep a storage map with all updated sum of ledger.unlocking for all ledgers.