search

logos-co / staking

SNT Staking contracts
Creative Commons Zero v1.0 Universal
0 stars 4 forks source link

Epoch `totalSupply` is not properly updated retroactively #91

Open 0x-r4bbit opened 4 months ago

0x-r4bbit commented 4 months ago

Here's an issue I ran into while writing/updating tests for #14:

UPDATE: I realized, while writing this down, it's essentially the same issue described in https://github.com/logos-co/staking/issues/48

The staking contracts keep track of two things - epochs and accounts.

Epochs can be advanced by calling executeEpoch(). This will calculate the overall pendingRewards as well as the epochRewards for the current epoch and the totalSupply of the current epoch. executeEpoch() is also called via modifiers in any state changing operation (stake(),unstake()` etc)

Epochs can advance without stakers having calculated their epoch rewards. This is done so that when executeEpoch() is called, an account doesn't have to process the rewards for all accounts of the system, but only its own.

In other words, it's very possible to have a state where, say, the currentEpoch: 25 and accountOneEpoch: 1 and accountTwoEpoch: 10. Every account has to take care of getting their rewards processed for every epoch.

If accounts happen to not do that for any given epoch, then these will be part of the pendingRewards.

Here's the problem

Because an account can process its rewards at a time when currentEpoch is higher than the accountEpoch, we need to update an epochs totalSupply retroactively once the rewards for that account have been calculated.

Right now this is not happening for all epochs between accountEpoch + 1 and currentEpoch.

Concrete example:

Let's pause here for a second. Notice that the account has processed its "own" epoch 1, so it's now at epoch 2. Due to the processing, it has calculated its rewards and the totalSupply of epoch one has been updated accordingly.

The totalSupply of the other epochs are left untouched, until the account processes its rewards for those epochs.

Let's continue:

And this is where we run into underflow bugs.

When we executeAccount() we're always operating on the state of the current epoch. The moment we call executeAccount(3) we're operating on epoch 2 which at this point in time, still only has the totalSupply without all the MP calculation that have happened in the meantime.

So what actually needs to happen, is that when we caculate the rewards for a given account and a given epoch, we need to update all following epochs until the latest one as well.

This problem only gets more complex the more stakers enter the system.

Here are some more concrete logs that show the same issue:

Logs:
  Processing epoch...
  --- currentEpoch:  0
  --- epochReward:  0
  --- totalSupply:  0
  --- pendingReward:  0
  Done. New epoch started:  1

  --- Staking:  10000000
  --- Minting MP:  10000000
  --- Adding revenue:  10000000000000000000
  Processing epoch...
  --- currentEpoch:  1
  --- epochReward:  10000000000000000000
  --- totalSupply:  20000000
  --- pendingReward:  10000000000000000000
  Done. New epoch started:  2

  --- Adding revenue:  10000000000000000000
  Processing epoch...
  --- currentEpoch:  2
  --- epochReward:  10000000000000000000
  --- totalSupply:  20000000
  --- pendingReward:  20000000000000000000
  Done. New epoch started:  3

  Processing account:  0x75537828f2ce51be7289709686A69CbFDbB714F1
  --- processing account epoch:  1
  --- userSupply in epoch (before):  20000000
  ------ account.balance:  10000000
  ------ account.currentMP:  10000000
  --- epoch totalSupply (before):  20000000
  ------ Minting multiplier points for epoch...
  --------- increasedMP:  191780
  --- userSupply in epoch (after):  20191780
  ------ account.balance:  10000000
  ------ account.currentMP:  10191780
  --- epoch totalSupply (after):  20191780
  --- userEpochReward:  10000000000000000000
  --- removing epoch userSupply from epoch totalSupply:  20191780
  --- New epoch epochReward:  0
  --- New epoch totalSupply:  0

  Processing account:  0x75537828f2ce51be7289709686A69CbFDbB714F1
  --- processing account epoch:  2
  --- userSupply in epoch (before):  20191780
  ------ account.balance:  10000000
  ------ account.currentMP:  10191780
  --- epoch totalSupply (before):  20000000.   <-- this should be the same as totalSupply (after) of prev epoch
  ------ Minting multiplier points for epoch...
  --------- increasedMP:  191780
  --- userSupply in epoch (after):  20383560
  ------ account.balance:  10000000
  ------ account.currentMP:  10383560
  --- epoch totalSupply (after):  20191780
  --- userEpochReward:  10094979244029005862
mart1n-xyz commented 4 months ago

I can see the problem. Part of it is the fact that other users' rewards in later epochs are a function of MPs that may not have been minted yet. With @3esmit, we thought we solved this with the estimating of aggregate MP within epoch execution (which you do not apply in the example) that is gradually corrected as users process accounts in this epoch (due to hitting max MP limit). However, I can see now that does not solve it entirely and leads to this inconsistency where we update the past but not the present that is a function of it.

We didn't realize users may process only up to a certain past epoch. As you point out, the next epochs' estimates then do not reflect this correction. If they process up to the current/latest epoch, the estimate correction should apply to all epochs and we are good.

0x-r4bbit commented 4 months ago

At this point I'm trying to revisit, why we needed epochs in the first place, or if we can get away with not having them. This would remove the problem entirely.

Or.. enforcing users to always processAccount() for all accounts when processEpoch() is done. In other words, the protocol wouldn't advance its epoch, without also calculating and distributing rewards for all accounts up to the current epoch. (which is potentially many transactions and expensive)