Re-run the tzkt tests #501

nicolasochem · 2021-09-24T18:29:35Z

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.

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

name: Pull Request
about: Create a pull request to make a contribution
labels:

IMPORTANT NOTICE:
I read and understood the guidelines for contributions to the TRD. The contribution may qualify for being compensated by the TRD grant if approved by the maintainers.

This PR resolves the issue . The following steps were performed:

Analysis: If the described issue is a bug report, analyze the reasons resulting in this bug.
Solution: Describe the proposed solution for the bug or feature.
Implementation: Rough description/explanation of the implementation choices.
Performed tests: Describe the performed tests.
Documentation: Make sure to document the added changes in a proper way (Readme, help section, documentation, comments in code if needed)
Check list:

I extended the Github Actions CI test units with the corresponding tests for this new feature (if needed).
I extended the Sphinx documentation (if needed).
I extended the help section (if needed).
I changed the README file (if needed).
I created a new issue if there is further work left to be done (if needed).

Work effort: Give your estimate of the work effort in hours. This might be adjusted or discussed by the other contributors in order to keep a fair rewarding process for the efforts.

@m-kus

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

tests/integration/test_tzkt_reward_api.py

tests/integration/tzkt_data/tz1S8e9GgdZG78XJRB3NqabfWeM37GnhZMWQ_235_expected.json

* do not store current balance: useless * typos * assertNotEqual becomes assertEqual

nicolasochem · 2021-10-21T23:37:06Z

@vkresch ready for re-review

vkresch · 2021-10-28T14:32:34Z

Thanks for the fixes and investigations!

Review Effort: 1h

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 anything to TRD 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 #415 changed the tzkt code to not do it. We agreed to make it configurable. This PR does it. It 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 returning only one value, it now returns several values (rewards from baking, endorsing, fees and revelations; accusation rewards; double baking losses). Various providers populate the same fields with custom logic. We end up with the below core logic executed independly of the provider chosen: ``` 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards` - it will leave these fields to "None". 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.

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 anything to TRD 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. This PR does it. It 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 returning only one value, it now returns several values (rewards from baking, endorsing, fees and revelations; accusation rewards; double baking losses). Various providers populate the same fields with custom logic. We end up with the below core logic executed independly of the provider chosen: ``` 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards` - it will leave these fields to "None". 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.

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 anything to TRD 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. This PR does it. It 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 returning only one value, it now returns several values (rewards from baking, endorsing, fees and revelations; accusation rewards; double baking losses). Various providers populate the same fields with custom logic. We end up with the below core logic executed independly of the provider chosen: ``` 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards` - it will leave these fields to "None". 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.

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. This PR does it. It 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`. The provider code populates these fields but no longer perform 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards` - it will leave these fields to "None". 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.

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. This PR does it. It 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`. The provider code populates these fields but no longer perform 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, RPC provider will populate `total_reward_amount` but not `rewards_and_fees` or `accusation_rewards`. It will leave these fields to "None". 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.

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. This PR does it. It 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`. The provider code populates these fields but no longer perform 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` The following is true: ``` total_reward_amount = rewards_and_fees + accusation_rewards - equivocation_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, 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.

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. This PR does it. It 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`. The provider code populates these fields but no longer perform 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 total_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: total_reward_amount = reward_model.total_reward_amount else: # omit denunciation rewards total_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: total_reward_amount = reward_model.total_reward_amount + reward_model.offline_losses else: # omit denunciation rewards and double baking loss total_reward_amount = reward_model.rewards_and_fees + reward_model.offline_losses ``` The `calculate` function now takes an extra arg `total_reward_amount` because this value is now controller by payment_producer and is no longer equivalent to `total_reward_amount` in the model. The following is true: ``` total_reward_amount = rewards_and_fees + accusation_rewards - equivocation_losses ``` payout_accusation_rewards is the new configuration option. It defaults to false, but you must set it to true if you use RPC provider otherwise you get an error: RPC can't distinguish between an accusation reward and a baking reward unless you parse every block (which is the job of an indexer). In other terms, 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.

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`. 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 perform 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.

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.

vkresch · 2021-11-13T08:48:26Z

@nicolasochem should this PR be merged before #509?

nicolasochem · 2021-11-13T17:20:33Z

Yes

…

On Sat, Nov 13, 2021 at 12:48 AM Viktor ***@***.***> wrote: @nicolasochem <https://github.com/nicolasochem> should this PR be merged before #509 <#509> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#501 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAWXC7BRQ77HAV3W6R6D4DULYQ6LANCNFSM5EWQB3FA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

nicolasochem requested review from m-kus and utdrmac September 24, 2021 18:29

lint

8cea09e

jdsika assigned vkresch Sep 30, 2021

jdsika linked an issue Oct 8, 2021 that may be closed by this pull request

tests should not depend on external apis being up #421

Closed

jdsika added this to the v10.0 (Granada) milestone Oct 8, 2021