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

consensus query for ledger reward provenance #2830

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

JaredCorduan
Copy link
Contributor

@JaredCorduan JaredCorduan commented Dec 21, 2020

This commit adds a new consensus query which returns a lot of data surrounding the reward calculation in the ledger. There is data concerning the overall calculation, but also detailed information about each individual stake pool's rewards and performance.

I updated the ledger package to master in order to get the CBOR instances for the reward provenance.

Resolves: CAD-2387

@@ -285,6 +293,7 @@ querySupportedVersion = \case
GetLedgerTip -> (>= v1)
GetEpochNo -> (>= v1)
GetNonMyopicMemberRewards {} -> (>= v1)
GetRewardProvenance -> (>= v2)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrBliss is it okay to add this new query to version 2 like I've done? Is there anywhere else that I need to document it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, adding this new query requires adding a new version, as V2 is already out in the wild and does not include this new query 🙁. If you plan on adding another query soon, I would try to get it in before this version is released, otherwise yet another version will be needed.

I'll push a commit to this PR to show what needs to be done to add a new version. It can be used as a template for the next queries. cc: @nfrisby.

FYI, the new version is needed because nodes using V2 are already released and they are not aware of the new GetRewardProvenance query. So when they receive such a query, they will fail to decode it and disconnect. The sender of the query will be left in the dark. By adding a new version and requiring it for the new query (done in this function), the sender of the new query will see an error when sending it to a node that doesn't support the new version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thank you for the explanation!

Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JaredCorduan Thanks for doing the work!

I'll push some fixes to this PR.

Comment on lines 89 to 90
GetRewardProvenance
:: Query (ShelleyBlock era) (SL.RewardProvenance (EraCrypto era))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move it to the bottom of the list so that it is chronological, which I think is useful in querySupportedVersion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree, that's nicer.

Comment on lines 215 to 216
sameDepIndex GetRewardProvenance _
= Nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also need a case for a match:

  sameDepIndex GetRewardProvenance GetRewardProvenance
    = Just Refl

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 160 to 161
GetRewardProvenance ->
snd $ SL.getRewardInfo globals st
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this one to the bottom too, for consistency? (Same for all other places you had to touch)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -285,6 +293,7 @@ querySupportedVersion = \case
GetLedgerTip -> (>= v1)
GetEpochNo -> (>= v1)
GetNonMyopicMemberRewards {} -> (>= v1)
GetRewardProvenance -> (>= v2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, adding this new query requires adding a new version, as V2 is already out in the wild and does not include this new query 🙁. If you plan on adding another query soon, I would try to get it in before this version is released, otherwise yet another version will be needed.

I'll push a commit to this PR to show what needs to be done to add a new version. It can be used as a template for the next queries. cc: @nfrisby.

FYI, the new version is needed because nodes using V2 are already released and they are not aware of the new GetRewardProvenance query. So when they receive such a query, they will fail to decode it and disconnect. The sender of the query will be left in the dark. By adding a new version and requiring it for the new query (done in this function), the sender of the new query will see an error when sending it to a node that doesn't support the new version.

@mrBliss
Copy link
Contributor

mrBliss commented Dec 22, 2020

I'll push some fixes to this PR.

I will probably force-push, so that the history is clean (and each commit is buildable).

mrBliss and others added 3 commits December 22, 2020 13:03
This commit adds a new consensus query which returns a lot of data
surrounding the reward calculation in the ledger. There is data
concerning the overall calculation, but also detailed information about
each individual stake pool's rewards and performance.

As this is a new query, a new version number is needed.
@mrBliss
Copy link
Contributor

mrBliss commented Dec 22, 2020

@nfrisby Adding you as a reviewer, as you will be doing this in the future 🙂.

Unfortunately, adding new queries requires adding a new version. See the the last commit for a template on what to do. The golden test results are generated automatically by running the golden tests.

@JaredCorduan
Copy link
Contributor Author

I'll push some fixes to this PR.

I will probably force-push, so that the history is clean (and each commit is buildable).

Thank you for cleaning this up for me @mrBliss !

@mrBliss mrBliss self-requested a review December 22, 2020 14:22
Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 22, 2020

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.

2 participants