Add GetRewardInfoPools local state query

[HeinrichApfelmus]

Issue number

CAD-3479

Summary

Introduces a new local state query GetRewardInfoPools which provides up-to-date information the stake pool rewards and ranking.

[KtorZ]

Isn't this one a bit redundant with GetRewardProvenance 🤔 ?

[HeinrichApfelmus]

Isn't this one a bit redundant with GetRewardProvenance 🤔 ?

Unfortunately, it's not redundant enough. 😅 Specifically,

• The GetRewardProvenance query returns the stake distribution and pool parameters from the snapshot labeled "go". At the time of query execution, this snapshot is two epochs old.
• The new GetRewardInfoPools query returns the current stake distribution and pool parameters*. To make an informed delegation choice, this information is required, as only this information pertains to the current epoch.

* "current" pool parameters (cost, margin) include any parameter changes from the previous epoch, but not from the current epoch.

[KtorZ]

Well, that's unfortunate 😅...

Although, as far as I remember, the main reason for introducing GetRewardProvenance was to give local clients means to make educated delegation choices. So perhaps the semantic of this query needs to be revised 🤔?

[HeinrichApfelmus]

So perhaps the semantic of this query needs to be revised 🤔?

Maybe, but changing the query semantics would not be backwards compatible (for example, the data types differ). My thinking was that adding a new query and deprecating the old one later would be the least disruptive path to getting a query with suitable semantics.

[nfrisby]

Please also add an entry atop the https://github.com/input-output-hk/ouroboros-network/blob/master/ouroboros-consensus/docs/interface-CHANGELOG.md file. Thanks!

[nfrisby]

We reviewed the use of versions here during a recent Consensus Planning call. I think the PR looks good.

@HeinrichApfelmus What's the status here? Do you have a plan for the CI failures?

I'm just double-checking that you're not blocked on us. Thanks.

[nfrisby]

Also, we merge with bors and it includes the PR description in the resulting merge commit. So would you please remove the "Comments" section from the PR description? (Maybe relocate it as a quote in a reply to this comment for posterity's sake.)

[nfrisby]

Lastly (I'm sorry for the comment barrage), we just merged a PR that updates the cardano-ledger-specs dependency. So you'll likely need to rebase onto master and then you'll be able to drop at least one of your commits (the want one that handles a deprecation warning from c-l-s).

[HeinrichApfelmus]

So would you please remove the "Comments" section from the PR description? (Maybe relocate it as a quote in a reply to this comment for posterity's sake.)

For posterity here the comments that were originally in the PR description:

Comments

• This feature depends on the new function getRewardInfoPools introduced in: Ledger API function for stake distribution and pool parameters query cardano-ledger#2511
• I have no idea which version numbers are appropriate. As instructed by the comments in the source code, I have attempted to follow the template given in consensus query for ledger reward provenance #2830 . However, I do not know which version is going to be released when, please adjust as necessary.

[HeinrichApfelmus]

What's the status here? Do you have a plan for the CI failures?

we just merged a PR that updates the cardano-ledger-specs dependency. So you'll likely need to rebase onto master and then you'll be able to drop at least one of your commits

Great, thanks! The merge of the PR from cardano-ledger-specs is essentially what I was waiting for. There is little point in appeasing CI if the main function to be called does not exist yet. 😄

[HeinrichApfelmus]

@nfrisby CI is now green. From my perspective, this PR is ready to be merged. 😊

If you merge this at the same time as #3404 , then you only have a single bump to NodeToClientV_11.

[dnadales]

Would it make sense to refer to what these new queries are?

[HeinrichApfelmus]

I figured that since comments are particularly susceptible to bitrot, it would be best if I mention the specific queries in a single place only, in this case the definition of ShelleyNodeToClientVersion5.

But I you prefer, I'm happy to the specific query (GetRewardInfoPools) at the definition of CardanoNodeToClientVersion8 as well.

[dnadales]

I figured that since comments are particularly susceptible to bitrot, it would be best if I mention the specific queries in a single place only, in this case the definition of ShelleyNodeToClientVersion5.

Sure, that makes sense. I'm ok with the comment as is.

[dnadales]

LGTM!

[nfrisby]

@HeinrichApfelmus Please issue bors merge whenever you're content with the commits (eg after one final rebase, etc). Thanks!

[HeinrichApfelmus]

bors merge

[iohk-bors]

Build succeeded:

• ci/hydra:Cardano:ouroboros-network:required
• buildkite/ouroboros-network