Refactoring + accusation payout toggle #509
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As I was working on some refactoring, I took a look at the tzkt integration tests. It appears that the tzkt tests are "cached" because they take one hour to run. Having discovered that, I then re-ran the tests. To do that, I deleted the cached data in tests/integration/tzkt_data/* I discovered that I actually broke the tests in #415 Previously, tzkt was paying out denunciation rewards. We have changed this default behavior to not pay them. The tzkt test compares the actual rewards from tzkt with the actual rewards from rpc. But rpc actual rewards (calculated by the protocol) take into account denunciation rewards. So we now have a discrepancy. To solve it, I am hardcoding the reward value that we expect, for these 2 tests where there is a double baking gain. This is a band-aid. We have several problems: * tests should not be cached. What should be cached is the tzkt api calls so they return faster than the real things (we talked about that before) * these tests should be generalized to tzkt, tzstats and api * the cycles we are testing against are pre-granada. we should replace with more recent interesting cycles Also there is a test mysteriously named "test staking balance issue" which was also failing. I assume it is a reference to [this](https://baking-bad.org/blog/2020/06/11/we-were-calculating-staking-balances-wrong/). This test attempts to match total staking balance value from rpc and tzkt in a cycle where the snapshot index was 15. An "assert_not_equal" test was previously passing and is now failing, which means that some values we expected not to match are now matching. Not sure how to interpret this! I removed this statement. @m-kus you wrote these tests and @utdrmac you are familiar with the "staking balance issue", perhaps you can make more sense of this test than I? Other changes * added comments so the next person is less confused than I was * indent the json cached data
* do not store current balance: useless * typos * assertNotEqual becomes assertEqual
When someone double bakes or endorses, anyone can submit an on-chain proof of equivocation. The bakers that includes the proof gets rewarded with half of the security deposit of the faulty baker. It is important to note that equivocation has been very rare on Tezos so this PR does not change TRD's behavior in the vast majority of cases. Out of the discussions leading to PR #415 being merged, it has emerged that TRD pays out accusation rewards inconsistently: RPC reward model will rely on the on-chain calculation of rewards, which includes accusation rewards. Tzstats does not include accusation rewards. Tzkt used to, but #470 changed the tzkt code to not do it. We agreed to make it configurable. We introduce a new configuration option `payout_accusation_rewards`. When using configure.py, the default choice is "False". This PR has turned into a major refactoring: the logic to calculate rewards used to be isolated per provider: tzkt, tzstats, rpc would make their own calculations and return the reward amount via the rewards model. It makes more sense to have this calculation done at payment producer level. To make this possible, I extended the model. While it was previously retuning only one value `total_reward_amount`, it now returns several additional values such as `equivocation_losses`, `offline_losses`, `rewards_and_fees`, `accusation_rewards`. Here are the new variables for the payment_producer object model: ``` self.delegator_balance_dict = delegator_balance_dict self.delegate_staking_balance = delegate_staking_balance self.num_baking_rights = num_baking_rights self.num_endorsing_rights = num_endorsing_rights \# rewards that should have been earned, had the baker been online self.offline_losses = offline_losses \# total reward as recorded in-protocol self.total_reward_amount = total_reward_amount \# When using indexers, the total amount above can be itemized as follows: \# * baking rewards, fees, revelation rewards self.rewards_and_fees = rewards_and_fees \# * losses from double baking/endorsing self.equivocation_losses = equivocation_losses \# * rewards from denunciating other people's double baking/endorsing self.denunciation_rewards = denunciation_rewards ``` The provider code populates these fields but no longer performs calculation of the reward amount to be split. Instead, this calculation is performed by the below core logic which is provider-independent: ``` if rewards_type.isEstimated(): logger.info("Using estimated rewards for payouts calculations") total_estimated_block_reward = reward_model.num_baking_rights * block_reward total_estimated_endorsement_reward = reward_model.num_endorsing_rights * endorsement_reward computed_reward_amount = total_estimated_block_reward + total_estimated_endorsement_reward elif rewards_type.isActual(): logger.info("Using actual rewards for payouts calculations") if self.pay_denunciation_rewards: computed_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards computed_reward_amount = reward_model.rewards_and_fees - reward_model.equivocation_losses elif rewards_type.isIdeal(): logger.info("Using ideal rewards for payouts calculations") if self.pay_denunciation_rewards: computed_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss computed_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` The result of tris calculation is a new variable `computed_reward_amount` which is then passed to the `calculate` function as a new argument. Previously `calculate` was relying on `total_reward_amount` value in the model. The following is true: ``` total_reward_amount = rewards_and_fees + accusation_rewards - equivocation_losses ``` RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards`. It will leave these fields to "None". This explains why we left `total_reward_amount` in the model. While it can be derived by other fields with tzstats or tzkt, this is not possible with RPC. Extending the model required several changes to tests as well. Specifically tzkt tests continue to be a pain. In PR #501 we had hardcoded the accusation rewards. Now, with our extended model, we can compare apples to apples again, and I removed these hardcoded values. Configuration ------------- I modified configure.py to set the new flag. All flags are currently mandatory, so I did not change that, but anyone upgrading TRD will have to modify their config, which is not great. What does the team think? Should we default to false when the config flag is absent? Testing ------- I did some manual testing with the following baker: ``` baking_address: tz1WnfXMPaNTBmH7DBPwqCWs9cPDJdkGBTZ8 ``` On cycle 78. This baker was rewarded for denunciating a double baking on this cycle. I observed that the rewards were higher when the new setting was set to "True". I also observed that the rewards value was consistent between tzkt and tzstats.
|
This looks like something for a free weekend.... thank you for this contribution. It most definitely does remove some inconsistencies! |
|
Conflicts due to previous merge of fixed tzkt tests @nicolasochem can you resolve those. I try to organize reviews for the weekend |
|
Update: ETA for merge: 28th |
vkresch
requested changes
Nov 27, 2021
There was a problem hiding this comment.
In general the PR looks great. The documentation of the new feature is really good and detailed.
The grouping of the calculation to just one place instead of to each individual provider is a really good step and design choice.
I had just some minor comments and questions. Also the PR needs to be rebased based on the latest master. I do not want to do it myself because I would then rewrite some commit history which only the author should do.
I also realized that we should use a test coverage tool to detect which code is covered by our tests. But that's a story for another or next PR.
Effort: 3h
vkresch
reviewed
Nov 27, 2021
vkresch
approved these changes
Dec 4, 2021
|
I will create after the merge a follow up PR with a unit test for the computation function and the test coverage tool. |
|
@nicolasochem you have to put the long stories in the PR and only do precise texts as commit messages. It is sheer impossible for me to merge this with a useful comment :( AND you need to rebase in order to not have old comments from the old PRs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When someone double bakes or endorses, anyone can submit an on-chain
proof of equivocation. The bakers that includes the proof gets rewarded
with half of the security deposit of the faulty baker.
It is important to note that equivocation has been very rare on Tezos so
this PR does not change TRD's behavior in the vast majority of cases.
Out of the discussions leading to PR #415 being merged, it has emerged
that TRD pays out accusation rewards inconsistently: RPC reward model
will rely on the on-chain calculation of rewards, which includes
accusation rewards. Tzstats does not include accusation rewards. Tzkt
used to, but #470 changed the tzkt code to not do it. We agreed to make
it configurable.
We introduce a new configuration option
payout_accusation_rewards.When using configure.py, the default choice is "False".
This PR has turned into a major refactoring: the logic to
calculate rewards used to be isolated per provider: tzkt, tzstats, rpc
would make their own calculations and return the reward amount via the
rewards model.
It makes more sense to have this calculation done at payment producer
level. To make this possible, I extended the model. While it was
previously retuning only one value
total_reward_amount, it now returnsseveral additional values such as
equivocation_losses,offline_losses,rewards_and_fees,accusation_rewards.Here are the new variables for the payment_producer object model:
The provider code populates these fields but no longer performs
calculation of the reward amount to be split.
Instead, this calculation is performed by the below core logic which is
provider-independent:
The result of tris calculation is a new variable
computed_reward_amountwhich is then passed to thecalculatefunction as a new argument. Previously
calculatewas relying ontotal_reward_amountvalue in the model.The following is true:
RPC can't distinguish between an accusation reward and a
baking reward unless you parse every block (which is the job of an
indexer). RPC provider will populate
total_reward_amountbut notrewards_and_feesoraccusation_rewards. It will leave these fields to "None". Thisexplains why we left
total_reward_amountin the model. While it can bederived by other fields with tzstats or tzkt, this is not possible with
RPC.
Extending the model required several changes to tests as well.
Specifically tzkt tests continue to be a pain. In PR #501 we had
hardcoded the accusation rewards. Now, with our extended model, we can
compare apples to apples again, and I removed these hardcoded values.
Configuration
I modified configure.py to set the new flag. All flags are currently
mandatory, so I did not change that, but anyone upgrading TRD will have
to modify their config, which is not great. What does the team think?
Should we default to false when the config flag is absent?
Testing
I did some manual testing with the following baker:
On cycle
7874. This baker was rewarded for denunciating a double baking onthis cycle. I observed that the rewards were higher when the new setting
was set to "True". I also observed that the rewards value was consistent
between tzkt and tzstats.