Domains staking: Epoch transition and Pending unlocks

[vedhavyas]

This PR brings the following changeset

• Epoch transition for domains
• Pending unlocks for all operator and nominator withdrawals

The first 6 commits add the required functionality for domain epoch transition and Pending unlocks

The remaining two commits connect all the pieces introduced in the previous 6 commits

• aadc97d: Adds the final epoch transition for domain. This is currently unused and will be called once block tree is in
• f354309: Adds the unlocking of withdrawals when a consensus block number matches the unlock number.

Would suggest going commit by commit for this one

Closes: #1648

Code contributor checklist:

• I have read, understood and followed contributing guide

[liuchengxu]

Make sense to me in general.

[vedhavyas]

@liuchengxu for now, I have removed the Operator info from the state. I think your attack vector makes it more easy to bloat the state

[NingLin-P]

We may not need to update these 2 variables in every iteration if we introduce a remaining_stake and update it instead which is also more straightforward IMHO, non-blocker.

[vedhavyas]

the total_stake here will act as remaining_stake at the end. As for the total_shares, not sure what you mean here. We would still need track the remaining shares to calculate the nominator's stake. Am I missing something here ?

[NingLin-P]

What I mean was something like this:

        let total_shares = operator.total_shares;
        let total_stake = operator
            .current_total_stake
            .checked_add(&operator.current_epoch_rewards)
            .ok_or(TransitionError::BalanceOverflow)?;
        let mut remaining_stake = total_stake;

        let freeze_identifier = T::FreezeIdentifier::staking_freeze_id(operator_id);

        Nominators::<T>::drain_prefix(operator_id).try_for_each(|(nominator_id, nominator)| {
            let nominator_share = Perbill::from_rational(nominator.shares, total_shares);
            let nominator_staked_amount = nominator_share.mul_floor(total_stake);

            let locked_amount = T::Currency::balance_frozen(&freeze_identifier, &nominator_id);
            let amount_to_mint = nominator_staked_amount
                .checked_sub(&locked_amount)
                .unwrap_or(Zero::zero());

            // remove the lock and mint any gains
            ...

            // update pool's remaining stake
            remaining_stake = remaining_stake
                .checked_sub(&nominator_staked_amount)
                .ok_or(TransitionError::BalanceUnderflow)?;

            // remove any pending deposits for this nominator and then remove the lock
            ...

            Ok(())
        })?;

Currently, we updating total_stake and total_share and using them to calculate nominator_staked_amount at the same time, it is not straightforward to see the calculation is correct IMO.

[vedhavyas]

That is incorrect. You suggestion does not account for shares that are burned. If you don't update the shares, then nominator_share will be always be incorrect.

[NingLin-P]

What do you mean by burned shares? If there are 10 total shares and 2 nominators with 3 and 7 shares respectively, the nominator_share of them should be 3/10 and 7/10, right?

[NingLin-P]

Rounding loss can be captured by remaining_stake.

[vedhavyas]

but it is not captured when calculating the nominator share. If that is possible, please give me an example here

[NingLin-P]

The remaining_stake is initialized to total_stake said 100, for every nominator we deduct nominator_staked_amount (assume rounding loss is 0.1, the value will be 19.9, 29.9, and 49.9 respectively) from remaining_stake, thus after all deduction the remaining_stake left 0.3 which is the rounding loss.

[vedhavyas]

Yes that is what we get with the current implementation. But you are not calculating share_percentage of each nominator after every deposit Withdraw. You are assuming the share_percentage is not changing which is not true. Here we are calculating nominator's share_percentage after every withdrawal so that (total_shares_left/total_stake_remaining) are upto date instead of pre calculating them earlier to account for those small increments of percentage due to a rounding down in the previous withdrawal

[NingLin-P]

You are assuming the share_percentage is not changing which is not true.

The share_percentage is not (and should) changed even in current impl, the only calculation of share is total_shares = total_shares.checked_sub((&nominator.shares)) which does not bring any rounding loss.

But I do see the difference now, in the current impl nominators can get a cut of the rounding loss. In the above example, after the first nominator is processed, the total_stake becomes 80.1 and the second nominator can withdraw 80.1 * 3/8 while my proposal is 100 * 3/10 and leave all rounding loss to treasury.

[NingLin-P]

Why use the consensus block for this period? We need to use the domain block instead.

IMO, this locking period of withdrawal is introduced in case there are slashes after the user issue withdrawal, such that we can still deduct from the locking withdrawal. The slash happens if there is fraud proof challenging an invalid ER, and the challenge period of ER is measured by domain block (i.e. BlockTreePruningDepth).

[vedhavyas]

No, this is per spec where once its moves to unlocking state, it is tracked with consensus chain block

[NingLin-P]

Seems it takes only StakeEpochDuration domain blocks (currently set to 5) to move into the unlock state, which is much smaller than BlockTreePruningDepth (currently set to 256).

[liuchengxu]

I believe the parameterization will be revisited once the block tree PR lands, should be a non-blocker for this PR.

[NingLin-P]

My point is not about the parameterization, StakeEpochDuration is expected to be small while BlockTreePruningDepth is expected to be large.

My concern is in the current implementation a malicious operator can submit a fraudulent ER and then deregister immediately, so they can successfully get away from slashing.

[NingLin-P]

Would be great to see concrete example TBH.

In either scenario of domain stall, network delay, and too many bundles to fit in one consensus block, can cause no domain bundle included in a consensus block thus no domain block will be driven from that consensus block, so the consensus chain will progress faster than the domain chain, which means the unlocking period (using consensus block) will progress faster than the challenging period (using domain block).

The following example uses BlockTreePruningDepth = 256, StakeWithdrawalLockingPeriod = 100 and StakeEpochDuration = 5 as the current implementation:

• In consensus chain #10 and domain chain #10, a malicious operator submits a fraudulent ER and a deregister request.
• After consensus chain progress #10 -> #12 and domain chain progress #10 -> #12, the domain epoch transited the deregistration and reached the unlocking state.
• The domain chain stalled at #12, but the consensus chain progressed as usual #12 -> #112, and the deregistration was unlock
• The consensus chain receives a fraud proof (which is acceptable until domain block #266) but can not slash the malicious operator as it is already deregistered.

[NingLin-P]

My issue with using domain blocks is that we would need to track every domains block height in the unlocks list then. Is that a valid concern?

I think it won't bring extra cost compared to the current approach, we just need to change the index of the unlock list from consensus_block_number to (domain_id, domain_block_number) unless I miss something.

[vedhavyas]

I think it won't bring extra cost compared to the current approach, we just need to change the index of the unlock list from consensus_block_number to (domain_id, domain_block_number) unless I miss something.

Not just that, but we also need to adjust the other Storage items like PendingWithdrawals. This will be also be scoped such that each with withdrawal is on FIFO order. May not be such a trivial change since the underlying assumption has changed.

StakeEpochDuration = 5

This is not the final number and has a TODO to revisit this number.

Looking at the example there, I dont see that practically but this was the known concern during our offline discussion.
I do not have a strong opinion here but we need to update spec accordingly once we make the decision since calling it Global pending unlocks is confusing as well.

The change will be handled as either a separate PR or part of the epoch PR that uses block tree.

[NingLin-P]

This will be also be scoped such that each with withdrawal is on FIFO order.

The domain block number is also monotonically increasing as the consensus block, thus this is preserved.

StakeEpochDuration = 5

This is not the final number and has a TODO to revisit this number.

No matter how large this number can be the malicious operator can always choose to attack at the last few blocks of the current epoch, that is why I use 12 in the above example instead of 15.

The change will be handled as either a separate PR or part of the epoch PR that uses block tree.

I do think it is an issue we need to address but I'm okay with this.

[vedhavyas]

I do think it is an issue we need to address

Aboslutely, please start the discussion on the spec v2 regarding or open existing one. Thanks!

[liuchengxu]

Looks reasonable to me.

[liuchengxu]

I believe the parameterization will be revisited once the block tree PR lands, should be a non-blocker for this PR.