Exclude retired pools from the `ListStakePools` API function result. #1945

jonathanknowles · 2020-07-24T07:54:14Z

Issue Number

#1937

Overview

This PR:

Applies a very simple filter to the Shelley ListStakePools API operation that excludes all pools with a retirement epoch earlier than or equal to the current epoch.
Adds an integration test to verify that if pool p has already retired, it will not be listed by ListStakePools.

Future Improvements

The current pool tracking implementation (which predates this PR) has two potential areas for improvement:

Reducing the number of database queries that need to be executed:
See: https://jira.iohk.io/browse/ADP-383
Performing garbage collection of retired pools from the database:
See: https://jira.iohk.io/browse/ADP-376

These are not tackled in this PR, but are instead recorded in the above tickets, so that we may schedule time to fix them at a later date.

…function.

…unction.

Add reminders to use more efficient database queries when generating the complete list of non-retired pools. See: https://jira.iohk.io/projects/ADP/issues/ADP-383

Generated by following the instructions here: https://docs.cardano.org/projects/cardano-node/en/latest/stake-pool-operations/node_keys.html The pool id appears to be arbitrary.

Unfortunately, Haddock doesn't support documentation comments here. So we make these comments into ordinary Haskell comments.

piotr-iohk

There is one problem I think. Playing around with the CI cluster I've observed that the pool scheduled to be retired in epoch n is still visible through the whole epoch n and disappears from the list in epoch n+1. While it is visible one can still join it but gets:

{"code":"no_such_pool"
,"message":"I couldn't find any stake pool with the given id: b45768c1a2da4bd13ebcaa1ea51408eda31dcc21765ccbd407cda9f2"}

I guess pool retired to be scheduled in n should not be shown from n onwards...

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs

lib/shelley/src/Cardano/Wallet/Shelley/Launch.hs

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

jonathanknowles · 2020-07-25T07:50:48Z

There is one problem I think. Playing around with the CI cluster I've observed that the pool scheduled to be retired in epoch n is still visible through the whole epoch n and disappears from the list in epoch n+1. While it is visible one can still join it but gets:
{"code":"no_such_pool"
,"message":"I couldn't find any stake pool with the given id: b45768c1a2da4bd13ebcaa1ea51408eda31dcc21765ccbd407cda9f2"}
I guess pool retired to be scheduled in n should not be shown from n onwards...

Nice catch!

I think I've found the source of this here:

https://github.com/input-output-hk/cardano-wallet/pull/1945/files#r460378458

I'll post a patch shortly.

EDIT: fixed in 6ec42fb

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/1945/files#r460378458

piotr-iohk

LGTM!

Corner case that may be considered in future PR/story:

pool A schedule to retire in n
wallet joins A in n-1 (shows as delegation next: n + 1, target: A)
pool disappears from the list in n (wallet still shows as delegation next: n + 1, target: A)
since n + 1 wallet shows as delegation active, target: A

jonathanknowles · 2020-07-25T08:36:15Z

Corner case that may be considered in future PR/story:
* pool A schedule to retire in n
* wallet joins A in n-1 (shows as delegation next: n + 1, target: A)
* pool disappears from the list in n (wallet still shows as delegation next: n + 1, target: A)
* since n + 1 wallet shows as delegation active, target: A

I agree we should definitely discuss what we want the GetWallet and ListWallets operations to show in the case where a pool has already retired, including the above corner case.

I've opened the following ticket to capture this discussion: #1949.

iohk-bors · 2020-07-25T09:17:42Z

Build succeeded

jonathanknowles added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jul 24, 2020

jonathanknowles added this to the (ADP-313) Track Stakepool Retirements milestone Jul 24, 2020

jonathanknowles self-assigned this Jul 24, 2020

jonathanknowles force-pushed the jonathanknowles/do-not-list-retired-pools branch 3 times, most recently from f96af60 to a983133 Compare July 24, 2020 09:24

jonathanknowles added 10 commits July 25, 2020 03:59

Extract out function listStakePools_ in Shelley API server.

5bfdcbc

Add EpochNo filter argument to Shelley listStakePools API server …

d77f32f

…function.

Pass the current epoch into the Shelley listStakePools API server f…

7dfb8a8

…unction.

Filter out retired pools from Shelley listStakePools API function.

ecb6014

Add reminders to use more efficient database queries.

7b8e3e2

Add reminders to use more efficient database queries when generating the complete list of non-retired pools. See: https://jira.iohk.io/projects/ADP/issues/ADP-383

Add new stake pool to Shelley integration test framework.

7f2b0c6

Generated by following the instructions here: https://docs.cardano.org/projects/cardano-node/en/latest/stake-pool-operations/node_keys.html The pool id appears to be arbitrary.

Make comments compatible with Haddock.

34b13f5

Unfortunately, Haddock doesn't support documentation comments here. So we make these comments into ordinary Haskell comments.

Add PoolConfig for a pool that will retire very early.

cde9814

Make it possible to sort StakePoolMetadata values into order.

d9057c3

Make the "contains pool metadata" test pass.

8063cf6

jonathanknowles force-pushed the jonathanknowles/do-not-list-retired-pools branch from a983133 to 8063cf6 Compare July 25, 2020 04:00

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Jul 25, 2020

Try #1945:

d6d2a2b

Add comments to testPoolConfigs to explain intention.

b443343

jonathanknowles marked this pull request as ready for review July 25, 2020 04:08

jonathanknowles changed the title ~~WIP: Do not list retired stake pools in the API.~~ Do not list retired stake pools in the API. Jul 25, 2020

jonathanknowles requested a review from rvl July 25, 2020 04:11

This comment has been minimized.

Sign in to view

jonathanknowles requested a review from piotr-iohk July 25, 2020 05:00

jonathanknowles mentioned this pull request Jul 25, 2020

Do not list retired pools in the API. #1937

Closed

2 tasks

piotr-iohk reviewed Jul 25, 2020

View reviewed changes

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/StakePools.hs Show resolved Hide resolved

lib/shelley/src/Cardano/Wallet/Shelley/Launch.hs Show resolved Hide resolved

jonathanknowles commented Jul 25, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved

Correct off-by-one error in function epochIsInFuture.

6ec42fb

In response to review feedback: https://github.com/input-output-hk/cardano-wallet/pull/1945/files#r460378458

jonathanknowles requested a review from piotr-iohk July 25, 2020 07:55

jonathanknowles changed the title ~~Do not list retired stake pools in the API.~~ Do not list retired stake pools in the ListStakePools API operation. Jul 25, 2020

jonathanknowles changed the title ~~Do not list retired stake pools in the ListStakePools API operation.~~ Exclude retired pools from the ListStakePools API function result. Jul 25, 2020

piotr-iohk approved these changes Jul 25, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jonathanknowles mentioned this pull request Jul 25, 2020

Decide how to show retired pools in the delegation field of ApiWallet. #1949

Closed

iohk-bors bot merged commit fe73ece into master Jul 25, 2020

iohk-bors bot deleted the jonathanknowles/do-not-list-retired-pools branch July 25, 2020 09:17

jonathanknowles mentioned this pull request Jul 25, 2020

There are un-joinable pools on shelley_testnet #1833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude retired pools from the `ListStakePools` API function result. #1945

Exclude retired pools from the `ListStakePools` API function result. #1945

jonathanknowles commented Jul 24, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

piotr-iohk left a comment

jonathanknowles commented Jul 25, 2020 •

edited

Loading

piotr-iohk left a comment

This comment has been minimized.

jonathanknowles commented Jul 25, 2020 •

edited

Loading

iohk-bors bot commented Jul 25, 2020

Exclude retired pools from the ListStakePools API function result. #1945

Exclude retired pools from the ListStakePools API function result. #1945

Conversation

jonathanknowles commented Jul 24, 2020 • edited Loading

Issue Number

Overview

Future Improvements

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

piotr-iohk left a comment

Choose a reason for hiding this comment

jonathanknowles commented Jul 25, 2020 • edited Loading

piotr-iohk left a comment

Choose a reason for hiding this comment

This comment has been minimized.

jonathanknowles commented Jul 25, 2020 • edited Loading

iohk-bors bot commented Jul 25, 2020

Build succeeded

Exclude retired pools from the `ListStakePools` API function result. #1945

Exclude retired pools from the `ListStakePools` API function result. #1945

jonathanknowles commented Jul 24, 2020 •

edited

Loading

jonathanknowles commented Jul 25, 2020 •

edited

Loading

jonathanknowles commented Jul 25, 2020 •

edited

Loading