livepeer / LIPs

Livepeer Improvement Proposals
9 stars 13 forks source link

LIP-36: Cumulative Earnings Claiming #35

Closed yondonfu closed 3 years ago

yondonfu commented 4 years ago

This is the discussion thread for LIP-36: Cumulative Earnings Claiming which proposes a more efficient earnings calculation algorithm that would reduce gas costs and improve the user experience of earnings claiming. The draft was recently merged. All feedback is welcome! All reviewers should refer to the LIP text for the latest version of the proposal.

dob commented 4 years ago

What do you think about including a community approved pendingStake snapshot as part of this LIP in order to make an O(1) cost claim txn available for all past rewards up until the block that this new go-forward method becomes active? My thought is that it could work as follows:

I know the scope of this may seem bigger and expansive of the algorithmic change, so perhaps it would be best proposed in a separate bundled LIP. But in light of the crazy high gas costs right now, it could unlocked 10's to 100's of $'s of LPT for thousands of users who currently can't claim without the gas fees being higher than the value they'd receive.

dob commented 3 years ago

Moved the discussion for the snapshot proposal into https://github.com/livepeer/LIPs/issues/52

yondonfu commented 3 years ago

@kyriediculous recently brought up two accounting edge cases:

Issue 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. This is because currently the cumulativeFeeFactor for round N is initialized when the first ticket is redeemed for round N and based on the cumulativeFeeFactor for round N - 1. So, if the cumulativeFeeFactor round N - 1 is updated afterwords when ticket 2 is redeemed, then the new value will not be reflected in the cumulativeFeeFactor for round N. As a result, any delegator that is claiming earnings starting from round N will not receive the fees for ticket 2.

Issue 2: If an orchestrator's cumulativeRewardFactor for round N - 1 is 0 and the orchestrator's lastRewardRound = currentRound then we are unable to initialize update the cumulativeFeeFactor for round N since it depends on the last known cumulativeRewardFactor. When lastRewardRound < currentRound, we could use the cumulativeRewardFactor for lastRewardRound, but that is not feasible in this case since lastRewardRound = currentRound.

My understanding of the proposed solution is the following:

Solution 1: When updateTranscoderWithFees(_round) is called by the TicketBroker (when a ticket is redeemed), instead of adding the fees to the earnings pool for _round (which corresponds to the ticket creationRound), add the fees to the earnings pool for the currentRound. This should address Issue 1.

Solution 2:

Use the logic described in the following pseudocode in a reward call:

if earningsPool[currentRound - 1].cumulativeRewardFactor == 0 {
    earningsPool[currentRound - 1].cumulativeRewardFactor = earningsPool[lastRewardRound].cumulativeRewardFactor
}

...

lastRewardRound = currentRound

Use the logic described in the following pseudocode in updateTranscoderWithFees():

earningsPool = earningsPool[currentRound]
prevEarningsPool = earningsPool[currentRound - 1]

if prevEarningsPool.cumulativeRewardFactor == 0 {
    if lastRewardRound < currentRound {
        cumulativeRewardFactor = earningPool[lastRewardRound].cumulativeRewardFactor
    } 
    // If lastRewardRound == currentRound prevEarningsPool.cumulativeRewardFactor should
    // have been set in the reward call for currentRound
} else {
    cumulativeRewardFactor = prevEarningsPool.cumulativeRewardFactor
}

This assume the use of Solution 1 and should address Issue 2.


Solution 1 seems like a reasonable way to simplify accounting so that we don't need to worry about fees being added to earnings pools in past rounds. I wasn't sure about this approach at first because I wanted to see if we could avoid changing how ticket fees are added to earnings pool in this LIP. But, after considering how this approach addresses Issue 1, I think changing how ticket fees are added to earnings pool is reasonable especially because it makes it easier to reason about accounting (no fees for past rounds).

Solution 2 does seem to address Issue 2. The one optimization suggestion I would make here is to omit the updated logic for reward calls and update the logic for updateTranscoderWithFees(_round) to:

earningsPool = earningsPool[currentRound]
prevEarningsPool = earningsPool[currentRound - 1]

if prevEarningsPool.cumulativeRewardFactor == 0 {
    if lastRewardRound < currentRound {
        cumulativeRewardFactor = earningPool[lastRewardRound].cumulativeRewardFactor
    } else {
        // Derive the cumulativeRewardFactor for currentRound - 1 based
        // on rewards for currentRound
        rewards = (minter.currentMintableTokens() * earningsPool.totalStake) / currentRoundTotalActiveStake
        // This assumes rewardCut is out of 100
        transcoderCommissionRewards = (rewards * earningsPool.rewardCut) / 100
        delegatorRewards = rewards - transcoderCommissionRewards

        cumulativeRewardFactor = earningsPool.cumulativeRewardFactor / (1 + (delegatorRewards / earningsPool.totalStake))
    }
} else {
    cumulativeRewardFactor = prevEarningsPool.cumulativeRewardFactor
}

The main benefit of this optimization is that it would avoid a storage update. WDYT?

kyriediculous commented 3 years ago

I like the optimisation very much as it saves quite a bit of gas, it achieves the same result while also falling under the constraint that that approach only works when implementing solution 1 as well.

Since we are in agreement about these changes I think we can move forward with the implementation of these changes in LIP-36.

kyriediculous commented 3 years ago

One thing to note:

The current API for updating fees is

function updateTranscoderWithFees(
        address _transcoder,
        uint256 _fees,
        uint256 _round
)

but this could indeed be simplified to

    function updateTranscoderWithFees(
        address _transcoder,
        uint256 _fees
    )

Which would entail changes the the TicketBroker as well (though minor) if we were to accommodate for this change in signature

The only real difference is saving on a very tiny amount of gas however so I'd be okay with keeping the API as is, what's your take on this?

yondonfu commented 3 years ago

Which would entail changes the the TicketBroker as well (though minor) if we were to accommodate for this change in signature

I think we should keep the same function signature for updateTranscoderWithFees() so that we can avoid upgrading the TicketBroker. I don't think the tiny gas reduction from updating the function signature is worth the extra operational burden from introducing an additional contract upgrade.

yondonfu commented 3 years ago

This LIP has just been assigned the Last Call status which kicks of a 10 day Last Call period for any remaining feedback prior to the LIP being assigned the Proposed status.

diogo-alcantara commented 3 years ago

Hi, I can't Claim my Earnings. Fail with error "too many rounds to claim through".

TXID:

https://cn.etherscan.com/tx/0xca328a9820cb93ca4347dc172a24e14ddb9e8f45bbe00366813d62f6013cfa2f

yondonfu commented 3 years ago

Hey @daf1984 while LIP-36 (described in this discussion thread) has been deployed meaning that claiming future earnings will no longer be subject to the "too many rounds to claim through" restriction, you will still be subject to this restriction for claiming past earnings which is why you encountered that error when you submitted your transaction. If you can wait, I suggest you vote in the LIP-52 poll https://explorer.livepeer.org/voting/0xe29f65a251affeecf3caca4a917e145229abf8f2 - if LIP-52 is accepted, then once it is deployed you will no longer be subject to the "too many rounds to claim through" restriction when claiming past earnings and it will be substantially cheaper to claim past earnings as well.

yondonfu commented 3 years ago

Closing this thread since LIP-36 has been deployed and marked Final.