tezos-reward-distributor-organization / tezos-reward-distributor

Tezos Reward Distributor (TRD): A reward distribution software for tezos bakers.
https://tezos-reward-distributor-organization.github.io/tezos-reward-distributor/
GNU General Public License v3.0
87 stars 51 forks source link

Paris support - fix #703 #704

Closed nicolasochem closed 1 month ago

nicolasochem commented 2 months ago

Update - 2024-06-26

The code may now be carefully used to pay rewards for cycle 749.

Do a dry-run first. Here is how to sanity-check your dry-run:

In the logs for your dry-run, you will see the following line:

INFO - Total rewards before processing is 100,000,000 mutez.

This value is no longer the total rewards. This is only the part of rewards attributable to delegation. You will get more rewards from staking.

This is not the same value as the one you can see in tzkt "rewards" page, which sums up up delegation and staking rewards. You can however check this value in the API by querying the following:

curl -s https://api.tzkt.io/v1/rewards/split/tz1xyz/749 | jq '(.blockRewardsDelegated + .endorsementRewardsDelegated) / 1000000'

(replace tz1xyz with your baker address)

A little below, in the logs, you will see the following:

Total estimated amount to pay out is 90,000,000 mutez.

This is what TRD will pay out.

As a public baker, most of your balance is probably staked (and if it is not, you should fix it by running the stake command). This has another effect: TRD no longer "carves out" some of the rewards for your own stake. Indeed, your own stake is paid for by the protocol, using the new mechanism.

Under AI, most of what TRD pays you is delegation fees. You can sanity check with the 2 values above: if the rewards attributable to delegation are 100 tez, and your fee is 10%, then you should expect TRD to pay out about 90 tez. This is assuming a normal config without any "owners" or "founders" set (I have not tested this). If the values you are seeing in the dry run are significantly outside of this, please reach out on slack.

The amount you will pay out at every cycle will probably be less than what you are used to paying prior to Adaptive Issuance activation. It's normal: in the new model, delegators will earn less, stakers earn more.

How this PR was tested

This PR (commit eaad513) was used to pay out rewards for cycle 749 for 2 small-ish mainnet bakers with no issues. One of them is accepting third-party stakers and the other is not. The values are looking good according to the rule of thumb above.

Caveat emptor:

  1. CI is failing. We are lacking workforce to fix it - in fact, the changes in Adaptive Issuance are of such magnitude that it might be better to start from an empty codebase and selectively import the python code that we still need, and write a new set of tests. This means, some things might be broken
  2. The current PR has not been tested in every configuration - especially no distinct "owners" or "founders". I am tempted to delete these features as well, but I don't see why AI would break it, so I left them. use at own risk.
  3. TRD accuracy is 100% downstream of tzkt accuracy. However, tzkt does not provide accurate data from the protocol, because this data is simply not accessible. Instead, it approximates it. This is discussed at length in this ticket. Specifially, the delegator balance is measured at the end of the cycle by tzkt. The protocol takes the minimum. This is potentially gameable by delegators.
  4. This new way of calculating delegating balance by picking the maximum is breaking smart contracts such as kolibri ovens, for which balance goes to zero when you pay them out. Therefore, paying out kolibri ovens lowers your delegation rewards for the cycle where the payout takes place. A fix is coming for protocol Q, allegedly. But for the time being, it's a good idea to not pay kolibri ovens, or pay them to the originator address rather than the contract iself. TRD has a feature for that, which I have not tested in a while. It's also worth noting that TezPay has implemented some middleware that addresses the problem, but we are not making use of it, due to lack of time, mostly.
  5. no one has reviewd my changes, yet (kiln.fi took a look at an earlier version and spotted some issues, which I now believe are addressed. thanks!)
  6. keep in mind that the license for the software says that we make no guarantee, implicit of explicit of proper operation, or suitability for a particular purpose.

What this PR does

Dev: 25 hours

vkresch commented 2 months ago

Looks good! The tests currently fail because of KeyError: 'ownDelegatedBalance'. We might want to record new requests for the tests which have those keys:

nicolasochem commented 2 months ago

Looks good! The tests currently fail because of KeyError: 'ownDelegatedBalance'. We might want to record new requests for the tests which have those keys:

  • ownDelegatedBalance
  • externalDelegatedBalance
  • delegatedBalance
  • currentDelegatedBalance

Can you please try?

vkresch commented 2 months ago

@nicolasochem is there any real api calls possible right now with those keys? Or will it only possible after Paris release? Can you link the documentation where these keys are mentioned? Thanks!

nicolasochem commented 1 month ago

@vkresch yes, from what I can tell it's all available on tzkt ghostnet & mainnet today.

API doc: https://api.tzkt.io/#operation/Rewards_GetRewardSplitDelegator

TPXP commented 1 month ago

I believe we should add ownStakedBalance to the delegate_staking_balance since the rewards we see on the baker address include rewards from the address's own stake. Did I miss something in my reasoning?

No, ownStakedBalance is not taken into account for delegation rewards. On TRD codebase we are still calling this variable delegate_staking_balance but it is a misnomer, it should be external_delegated_balance.

I'm not sure why your delegator rewards would be higher.

TPXP commented 1 month ago

Hello there, I see you updated my previous comment. Thanks for your reply!

I’ll elaborate since we tested this PR on one of our Ledger Bakers (tz3Vq38qYD3GEbWcXHMLt5PaASZrkDtEiA8D) for cycles 743 and 744.

One of our delegators is tz2Pi2Aoo4Dw2MYgLPiEUixG5ns5XtB4vmga. We send them rewards with TRD. As you can see in the operations log, rewards have been significantly higher for cycles 743 and 744 (those are the 2 lines above 2k tezos). Yet our rewards have been stable for the last 10 epochs.

Looking at calculation reports shows a significant decrease in OWNERS_PARENT for these cycles, which is computed from delegate_staking_balance (it substracts all staked balances). We added ownStakedBalance to the calculation: https://github.com/kilnfi/tezos-reward-distributor/commit/d40a6e71a90b138398e93dff8f44b2cdf6446cb0 which caused OWNERS_PARENT to be back to its previous value, and rewards are back to normal as can be seen in the latest payments (for cycles 745 and 746).

Here's what our calculations CSV files look like - I'm afraid I had to redact a few columns. Let me know if you need anything else :)

I'm not sure if the upgrade will change anything to the calculations, but I think we should handle pre-upgrade cycles as well?

PS: please comment instead of updating my comment, it makes it easier to follow the conversation and sends me a GitHub notification.

nicolasochem commented 1 month ago

@TPXP I didn't mean to edit your comment instead of replying, my bad.

This indeed looks like an important discrepancy.

delegate_staking_balance was populated by a deprecated field of TzKT stakingBalance which I believe represented "the total amount of tez owned by the baker and its delegators".

OWNERS_PARENT is a value derived from the balance of the baker only: currently, it is calculated by substracting total_delegator_balance from delegate_staking_balance: what's left is the bakers balance (free and staked)

Without AI, this makes sense as it counts equally towards rights and rewards.

However, with AI, this substraction no longer makes sense, as only the free balance of the baker and the delegators is taken into account for reward distribution. To give an extreme example, if the baker stakes their entire balance, none of the delegation rewards are theirs and theirs alone; they only take delegation fees.

This leads me to believe:

Therefore, your fix is valid before AI, but now, AI is active on mainnet, so it will not be accurate for next cycle.

The issue is, I have never touched the payment calculation code. I am having difficulties defining what the OWNER_PARENT variable is and what its purpose is. I don't see any useful comment in the code. @jdsika @vkresch can you please have a look, see if my reasoning is right.