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

Implement new Stake Pool Ranking formulas #3190

Merged
merged 7 commits into from
Mar 25, 2022

Conversation

HeinrichApfelmus
Copy link
Contributor

Issue number

ADP-1510, ADP-1491

Overview

In this pull request, we implement the new formulas for ranking stake pools. These comprise two numerical quantities and one notification message:

  • currentROS = The return of stake (annualized) expected when delegating to the pool right now.
  • saturationROS = The return of stake (annualized) expected when delegating to the pool if the pool were saturated and had 1/k of the total stake delegated to it, which is the ideal outcome for this pool in the future.
  • A redelegation warning that encourages the user to redelegate to a different pool if the performance of the current pool becomes lackluster. Such a notification is necessary because the above rankings do not take into account block production performance.

While this pull request implements the formulas, it does not yet apply them to actual blockchain data, or expose them via the REST API; this is subject to future pull requests closely related to PR #2781.

However, this pull request also includes code for estimate the pool performance from past pool production data, so that these formulas can also be used in light-mode (Epic ADP-1422). This code was mostly copied from the existing implementation in the cardano-ledger repository.

Details

  • Testing currentROS and saturationROS has been done manually, as there are no tests that are sufficiently independent of the definition.
  • Likewise, the redelegation warning is hard to test, but basic sanity checks are performed to detect typos.
  • Likewise, the performance estimation was tested manually on mainnet, and relies on past testing of the likelihood estimation code in the ledger.

Comments

  • The "redelegation warning" was originally called "redelegation notification". I somehow started calling it a "warning". Is that good UX or does that contribute to "warning fatigue"?
  • The present code does not yet consider warnings from past epochs. The specification is to make the warning sticky and also display it if it was triggered in one of the two previous epochs, but add a note in this case.
  • The present code also does not yet take into account parameter changes that affect pool performance across epochs.

@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1491/rewards branch 4 times, most recently from 92adb49 to 83bd65c Compare March 22, 2022 20:34
@HeinrichApfelmus
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Mar 24, 2022
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 24, 2022

try

Build failed:

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks great

lib/core/src/Cardano/Pool/Rank/Likelihood.hs Show resolved Hide resolved
lib/core/src/Cardano/Pool/Rank.hs Show resolved Hide resolved
lib/core/src/Cardano/Pool/Rank.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Pool/Rank.hs Show resolved Hide resolved
lib/core/src/Cardano/Pool/Rank.hs Show resolved Hide resolved

-- | Non-Myopic Pool Member Rewards
-- according to Eq.(3) of Section 5.6.4 in SL-D1.
nonMyopicMemberReward
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this one need review as well?

Copy link
Contributor Author

@HeinrichApfelmus HeinrichApfelmus Mar 25, 2022

Choose a reason for hiding this comment

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

Nope, no need to worry about that one. Will be phased out anyway, and was taken from / tested in the (now stale) PR #2781

lib/core/src/Cardano/Pool/Rank.hs Show resolved Hide resolved
lib/core/src/Cardano/Pool/Rank.hs Show resolved Hide resolved
returns = map (\i -> currentROS rewardParams i user) $ Map.elems pools

w = dt*dt / (25 + dt*dt) :: Rational
dt = fromIntegral $ fromEnum now - fromEnum timeOfDelegation
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't remember why we didn't put an Integral instance on EpochNo. Enum is a bit yucky.
Could this be written shorter like this?

dt = fromIntegral $ fromEnum $ now - timeOfDelegation

Copy link
Contributor Author

@HeinrichApfelmus HeinrichApfelmus Mar 25, 2022

Choose a reason for hiding this comment

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

I'd love to, but EpochNo is a newtype on Word31, i.e. an unsigned integer. When testing unrelated stuff on checkpointing, I came to the conclusion that I should avoid the combination of unsigned integers and substraction like the plague. I'm using fromEnum because it gives a signed Int here, and then I feel more confident to subtract.

@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1491/rewards branch from 3e9131e to 8e8596a Compare March 25, 2022 12:27
@HeinrichApfelmus HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1491/rewards branch from 8e8596a to 992d3af Compare March 25, 2022 12:41
@HeinrichApfelmus
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 25, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 29a4507 into master Mar 25, 2022
@iohk-bors iohk-bors bot deleted the HeinrichApfelmus/ADP-1491/rewards branch March 25, 2022 18:15
WilliamKingNoel-Bot pushed a commit that referenced this pull request Mar 25, 2022
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