search

livepeer / protocol

Livepeer protocol
MIT License
154 stars 45 forks source link

LIP-36: Cumulative earnings calculation #390

Closed kyriediculous closed 3 years ago

kyriediculous commented 4 years ago

This PR implements cumulative earnings calculations as described in https://github.com/livepeer/LIPs/blob/master/LIPs/LIP-36.md

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build d8959c92-38b5-4da2-9b84-7247b032f395

Totals Coverage Status
Change from base Build 81b62911-8428-461a-a022-1a61321d67d4: -0.09%
Covered Lines: 760
Relevant Lines: 760
💛 - Coveralls
kyriediculous commented 4 years ago

Considerable changes compared to the original spec:

  1. If an orchestrator redeems ticket 1 with creationRound = N and then ticket 2 with creationRound = N - 1, then the fees for ticket 2 would be lost.

solution Always claim fees for the current round https://github.com/livepeer/LIPs/issues/35#issuecomment-673659199

  1. If an orchestrator's cumulativeRewardFactor for round N - 1 is 0 and the orchestrator's lastRewardRound = currentRound then we are unable to initialize the cumulativeFeeFactor for round N since it depends on the last known cumulativeRewardFactor

solution Retroactively calculate the woud be rewardFactor for round N-1 and store it on the earningsPool for round N-1 https://github.com/livepeer/LIPs/issues/35#issuecomment-673659199

  1. If an orchestrator calls reward for round N but has missed a reward call for round N-1 we can not correctly calculate rewards

solution If prevEarningsPool.cumulativeRewardFactor == 0 set it to the cumulativeRewardFactor of the earningsPool for the orchestrator's lastRewardRound

  1. If the transcoder receives fees for round N But didn't receive fees for round N-1 then prevEarningsPool.cumulativeFeeFactor will not be set and we lose fees for all rounds prior to round N-1

solution Add a new state variable to the transcoder struct: lastFeeRound Set lastFeeRound when receiving fees If prevEarningsPool.cumulativeFeeFactor (for round N-1) is 0 write the cumulativeFeeFactor for the lastFeeRound to that pool instead.

  1. If a transcoders last claimed in round N and now tries to claim for round N +1 through N + 1 + Y it will not include fees for round N + 1

solution When using the cumulative approach for fees and rewards let the startEarningsPool be that for round N

  1. Cumulative fee calculation will always returns fees since the inception of cumulative fee calculation regardless of prior fees being claimed and withdrawn or not, which effectively leads to double spending fees

~solution~ ~(1) Track a new state variable on the delegator struct: pastFees When withdrawing fees , add the fees being withdrawn to pastFees When calculating cumulative fees substract pastFees~

~(2) Don't add delegator.fees to cumulativeFees when calculating fees for earnings when all rounds to claim for are following the cumulative calculation approach~

Update fee earnings calculation to

delegator.currentBondedAmount * (endCumulativeFeeFactor - startCumulativeFeeFactor) / startCumulativeRewardFactor

TODO

(basically add if earningsPool.hasTranscoderRewardFeePool { break (from loop); }

kyriediculous commented 4 years ago

There is a substantial change to the way updateDelegatorWithEarnings works.

In this PR we re-use pendingFees() and pendingStake() to calculate the amount of fees and rewards owed to the delegator that is claiming earnings.

This reusability of those two helpers shortens the code for updateDelegatorWithEarnings a fair bit.

Prior to this PR the only significant difference between using pendingFees and pendingStake is that EarningsPool.claimShare() is called for each earnings pool that earnings are being claimed for in updateDelegatorWithEarnings().

This function subtracts the claimed fees and rewards from the feePool and rewardsPool as well as subtract the delegators stake from claimableStake for the earningsPool.

IIUC correctly however there are no negative drawbacks to NOT doing these state updates as is in this PR. This does not affect earnings calculation.

ie. consider the following scenario:

400 in fees in earningsPool 2000 in claimable stake in earningsPool delegator1 has 500 in stake delegator2 has 1500 in stake

delegator 1 claims and receives 100 in fees ( 400 * (500/2000) ) -> remaining feePool = 300 , remaining claimableStake = 1500

delegator 2 claims and receives 300 in fees ( 300 * (1500/1500) ) -> remaining feePool = 0 , remaining claimableStake = 0_

If we don't do these state updates however the result remains the same

delegator1 receives 400 (500/2000) = 100 in fees delegator2 receives 400 (1500/2000) = 300 in fees

Double spending is already avoided by setting a delegator's lastClaimRound , the only thing these state updates really accomplish IIUC is to give a historical view of what is left in an EarningsPool but this seems rather unnecessary.

HOWEVER as a result of not doing these state updates we will never have an earningsPool for which earningsPool.hasClaimableShares() evaluates to false. These in fact are the two branches in coverage that are currently not being hit.

yondonfu commented 4 years ago

IIUC correctly however there are no negative drawbacks to NOT doing these state updates as is in this PR.

The reason the state updates for feePool, rewardsPool and claimableStake were included in the old earnings claiming algo implementation is that they allow additional fees and/or rewards added to the earnings pool for a round after other delegators have already claimed from the earnings pool to be distributed amongst the remaining delegators that have not yet claimed from that earnings pool. For example, in the scenario you outlined, consider the case where more fees are added to the earnings pool after delegator 1 claims. So, I think those state updates should be kept (and if LIP-52 is implemented then those state updates wouldn't need to be executed in practice) so that changes to the old earnings claiming algo can be kept out of this PR even if the behavior of distributing additional rewards/fees to delegators that have not yet claimed is not a part of the new earnings claiming algo.

kyriediculous commented 4 years ago

additional fees and/or rewards added to the earnings pool for a round after other delegators have already claimed from the earnings pool to be distributed amongst the remaining delegators that have not yet claimed from that earnings pool

I don't think this is the right approach either way. This is a UX problem and I've already expressed my opinion that UX should not be handled in the contracts if it involves several state reads and writes.

This could e.g. be handled by a notification in the explorer stating that you are trying to claim earnings up until a round your orchestrator has not called reward for and you will miss out on those earnings.

For fees it could be something similar, don't claim up until currentRound but currentRound - 1 unless one really needs all earnings to be claimed right now.

I also don't think that it's fair to redistribute those fees/rewards to the other delegators, I think it would be fine for those fees/rewards to just be considered lost as well (again, UX problem).

So, I think those state updates should be kept (and if LIP-52 is implemented then those state updates wouldn't need to be executed in practice) so that changes to the old earnings claiming algo can be kept out of this PR

I disagree on this point

The benefit of changing the approach here is that it allows for code readability and reusability with very minimal impact to the current behaviour. The only fees/rewards impacted would be those for the LIP-36 upgrade round prior to the upgrade and if and only if someone claims at the very beginning of that upgrade round, which is quite unlikely to happen.

After LIP-36 is deployed fees and rewards for those past rounds can't be added either way, so that code really has no use, regardless of LIP-52 being used or not.

Therefore I would be in favour of allowing for the reusability of pendingStake() and pendingFees() over an edge case for one round.

yondonfu commented 4 years ago

The benefit of changing the approach here is that it allows for code readability and reusability with very minimal impact to the current behaviour.

I understand. However, my concern is that reviewing these changes might become more challenging because now a third party is not just responsible for reviewing the correctness of the implementation of the new earnings claiming algo, but also the correctness of the changes to the old earnings claiming algo. Contrast this with the scenario where a third party reviewer would only need to be responsible for reviewing the correctness of the implementation of the new earnings claiming algo.

I'd be interested in understanding if there is an alternative way to achieve similar code readability and reusability benefits without expanding the scope of LIP-36 to also include changes to the old earnings claiming algo. Curious if you think that would be possible. I'll also keep this in mind as I jump into the actual code review at the beginning of next week.

kyriediculous commented 4 years ago

Okay yeah that's fair enough.

I'll duplicate the code for now then and we can see this as an implementation choice TBD in the review process.

I guess a middle road is to create helpers for just the cumulative calculations and re-use those.

kyriediculous commented 4 years ago

As expected though, duplicating the code puts us over the bytecode limit so we NEED reusable helpers.

but also the correctness of the changes to the old earnings claiming algo.

Since it's for a single round I actually feel like this is okay given the bytecode length hiccup as well. I also had to fight my way around stack depth before that.

Reusing pendingFees and pendingStake becomes a more reasonable tradeoff the further I dig.

UPDATE

I wrote two helpers , this puts us right under the bytecode length limit. 

build/contracts/BondingManager.json: 24549.5

The limit is 24577

Compared to reusing pendingFees and pendingStake however there is still a fair increase in bytecode length and I fear we will not be able to add the necessary logic for the snapshot claiming as is.

With the PR as is (reusing pendingFees and pendingStake) , the bytecode length is under 24000

UPDATE 2

I managed to get the bytecode length down a bit more 

build/contracts/BondingManager.json: 24386.5
kyriediculous commented 4 years ago

Contrast this with the scenario where a third party reviewer would only need to be responsible for reviewing the correctness of the implementation of the new earnings claiming algo.

After sleeping on it for the weekend I don't think we should let that drive our decision here. Highlighting the change should be quite sufficient in my opinion because it would only matter for the LIP-36 upgrade round itself. Any side effects are thus negligible.

kyriediculous commented 4 years ago

For reference here is the bytecode length reusing pendingFees and pendingStake

build/contracts/BondingManager.json: 23785.5

It's significantly smaller and gives us way more wiggle room to add logic for i.e. LIP-52

yondonfu commented 4 years ago

Compared to reusing pendingFees and pendingStake however there is still a fair increase in bytecode length and I fear we will not be able to add the necessary logic for the snapshot claiming as is.

I'm open to considering this change if necessary, although I'm interested in seeing where the bytecode size of the BondingManager stands after the requested changes are addressed first. Also, if we do make that change, using pendingFees() and pendingStake() as-is in updateDelegatorWithEarnings() will result in looping from startRound through min(endRound, LIP_36_ROUND) twice so there would likely need to be some refactoring to avoid that.