Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paris support - fix #703 #704

Merged
merged 14 commits into from
Jun 28, 2024
Merged

Paris support - fix #703 #704

merged 14 commits into from
Jun 28, 2024

Conversation

nicolasochem
Copy link
Contributor

@nicolasochem nicolasochem commented Jun 3, 2024

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

  • change sources for splitting up the delegation rewards
    • look at delegatedBalance instead of balance of bakers and delegators, as numerator
    • look at blockRewardsDelegated + endorsementRewardsDelegated as denominator
  • do not include bakers' fees as reward as discussed in issue
  • deprecate & remove code for "estimated" and "ideal" rewards. only "actual" rewards remain
  • remove code for rpc and tzstats data source. only tzkt remains
  • some doc updates
  • disable CI - no resource to maintain the testing code

Dev: 25 hours

Bare Minimum Barely tested code for now.

* look at `delegatedBalance` instead of `balance` in the TzKT API query
  results
* do not include bakers' fees as reward as discussed in issue
delegatedBalance is deprecated, need to sum up `own` and `external`
instead

also made a note that `currentDelegatedBalance` of delegators is also
deprecated
@vkresch
Copy link
Contributor

vkresch commented Jun 4, 2024

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

@nicolasochem
Copy link
Contributor Author

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
Copy link
Contributor

vkresch commented Jun 5, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

TPXP commented Jun 12, 2024

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
Copy link
Contributor

TPXP commented Jun 18, 2024

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: kilnfi@d40a6e7 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
Copy link
Contributor Author

nicolasochem commented Jun 21, 2024

@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:

  • the paris branch should not have been used to pay out rewards prior to AI activation: calculations were wrong
  • with AI activation, the phase 0 calculation code must change to adapt to the new model:
    • we need to pass the baker's own free balance, instead of calculating it, and use this for OWNERS_PARENT calculation

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.

TzKT offers own_delegated and external_delegated values. We pass them
both to the reward calculation function.

OWNER_PARENT is the baker's reward. It's named this way because it can
split into children rewards when the bakery has several "owners" and
"founders" (maybe we should also deprecate this functionality). Now, we
populate this value straight from tzkt.

We also use it to calculate the ratio. There is a sanity function that
ensures that the sum of itemized balances and the total from tzkt are
"almost equal". This apparently is still passing.

With these changes, I see more realistic values in my caculations:
values are lower than with main branch, which makes sense, because the
stake has now increased.

Also, for my baker, almost all balance is staked, so I only keep an
amount of delegation rewards approx. equal to my delegation fee. In
contrast, before AI, I would keep my fee plus my own rewards, which now
go almost exclusively to the staking balance.
@nicolasochem nicolasochem marked this pull request as ready for review June 28, 2024 05:05
@nicolasochem nicolasochem merged commit 091d406 into master Jun 28, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants