ADP-478: Garbage collect delisted stake pools from SMASH #2249

hasufell · 2020-10-15T15:52:14Z

This is the first step for garbage collecting
stake pools based on SMASH delisting.

X-JIRA-Ticket: ADP-478

Questions / Considerations

After looking at the logic, I believe we don't have to adjust any other functions of the DBLayer such as listRegisteredPools or readPoolProduction. These can still consider all pools. Only the API layer will consider delisted pools and adjust ApiStakePool accordingly.
There's still the open question whether we want to really delete GCed pools via removePools at some point.

TODO

Feature

Add delisted column to metadata table and populate Pool DBLayer with functions
Create new internal_state table and have GC thread store last sync time
Add garbage collection thread querying the SMASH server for delisted pools and calling delistPools from DBLayer
have GC thread store last sync time
return last GC sync time
Add POST '{ "maintenance_action": "gc_stake_pools" }' /stake-pools endpoint

QA

database tests
check json roundtripping works
Add integration tests? We don't have SMASH server integration tests yet.

lib/core/src/Cardano/Pool/DB/Sqlite.hs

hasufell · 2020-10-19T13:28:18Z

bors try

iohk-bors · 2020-10-19T14:13:33Z

try

Build failed:

ci/hydra-build:required

piotr-iohk · 2020-10-20T11:09:44Z

Add integration tests? We don't have SMASH server integration tests yet.

Perhaps we could have a mock SMASH server in the integration tests? Maybe it would be enough as it just mock the situation that there is one stake pool blacklisted. This way we could make a system level test.

Currently in our integration suite there are 4 pools being registered. One of them is retired at the very beginning (before the tests start). Two out of remaining three are a scheduled to be retired in the far future (and they are like that for the span of the suite). We could extend this set up to have yet another pool, but this one would served as "blacklisted" in the mock SMASH server and we would just make sure that this pool is not visible when we have metadata fetching strategy pointing to a mock SMASH server.

We used to have a mock server for jormungandr metadata registry, that was serving a zip file with metadata https://github.com/input-output-hk/cardano-wallet/blob/master/lib/jormungandr/test/integration/Main.hs#L230-L238... Maybe similar concept could be used.

Record delisted pools in a dedicated table instead of using a field in the `pool_registrations` table. In the updated schema, a pool is delisted if (and only if) there is a single row containing that pool's id in the `delisted_pools` table. This solution has several advantages: 1. We only need a single database row to record that a pool is delisted. 2. We no longer need to carefully to ensure that all registration records for a particular pool have the same delisted status. A pool is either delisted or not delisted: the schema rules out all intermediate states. 3. Pools automatically remain delisted when rollbacks occur or when new certificates are published, with no extra effort. 4. The `putPoolRegistration` function no longer needs to read the most-recently-written registration entry before adding a new entry. 5. Each row in the `pool_registrations` table is now just an immutable record of a registration certificate. 6. The `PoolFlag` type is no longer necessary.

In response to review feedback: #2277 (comment)

We must replace the set of delisted pools, rather than augmenting it. In response to review feedback: #2277 (comment)

This operation completely replaces the set of delisted pools, rather than augmenting it. Therefore, the name `delistPools` is slightly misleading, as it gives the impression that the existing set will be augmented, which is no longer true. In response to review feedback: #2277 (comment)

In particular, we check that 'putDelistedPools' completely overwrites the existing set every time. In response to review feedback: #2277 (comment)

This test no longer makes sense, as delisted pools are no longer stored in the `pool_registration` table. In response to review feedback: #2277 (comment)

In response to review feedback: #2277 (comment)

hasufell · 2020-11-10T09:44:28Z

bors try

iohk-bors · 2020-11-10T10:17:49Z

try

Build failed:

buildkite/cardano-wallet

It was a bit awkward to have two completely distinct endpoints for this with different paths, whereas they related to exactly the same thing. This commit makes the API a bit more consistent by wrapping the `gcPoolStatus` inside a `maintenance action` object. So that, the API now offers two dual endpoints: - post maintenance action - get maintenance action There's only one maintenance action available at this stage.

KtorZ

Final review done. Pushed some adjustment to the API layer to make it a bit more consistent and easier to follow from a user's perspective.

KtorZ · 2020-11-10T11:19:35Z

bors r+

hasufell · 2020-11-10T11:19:37Z

bors r+

iohk-bors · 2020-11-10T11:19:40Z

Already running a review

iohk-bors · 2020-11-10T11:55:43Z

Build succeeded:

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

@piotr-iohk

2309: Fix parsing of SMASHPoolId r=hasufell a=hasufell follow-up fix of #2249 found by @piotr-iohk https://smash.shelley-qa.dev.cardano.org/api/v1/delisted vs https://smash.cardano-mainnet.iohk.io/api/v1/delisted Co-authored-by: Julian Ospald <[email protected]>

hasufell requested a review from KtorZ October 15, 2020 15:52

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 865c6eb to 0ed2886 Compare October 15, 2020 16:12

hasufell changed the title ~~Add delistPools to Pool DBLayer~~ [WIP] Add delistPools to Pool DBLayer Oct 15, 2020

hasufell marked this pull request as draft October 15, 2020 16:22

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch 3 times, most recently from b298bb2 to c0dc034 Compare October 16, 2020 15:34

hasufell marked this pull request as ready for review October 17, 2020 14:32

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from b42eb40 to 9147481 Compare October 17, 2020 14:43

hasufell commented Oct 17, 2020

View reviewed changes

lib/core/src/Cardano/Pool/DB/Sqlite.hs Outdated Show resolved Hide resolved

hasufell changed the title ~~[WIP] Add delistPools to Pool DBLayer~~ Add delistPools to Pool DBLayer Oct 17, 2020

iohk-bors bot added a commit that referenced this pull request Oct 17, 2020

Try #2249:

e57ccf7

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch 2 times, most recently from a03670c to 6327f88 Compare October 17, 2020 16:43

cardano-foundation deleted a comment from iohk-bors bot Oct 17, 2020

iohk-bors bot added a commit that referenced this pull request Oct 17, 2020

Try #2249:

8e20ec9

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 6327f88 to 59a75fe Compare October 17, 2020 21:07

cardano-foundation deleted a comment from iohk-bors bot Oct 17, 2020

iohk-bors bot added a commit that referenced this pull request Oct 17, 2020

Try #2249:

eb84baf

iohk-bors bot added a commit that referenced this pull request Oct 19, 2020

Try #2249:

5e6160e

hasufell mentioned this pull request Oct 19, 2020

do not enforce non-empty outputs for unsigned tx #2255

Merged

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 59a75fe to 5d0c0a6 Compare October 19, 2020 13:28

iohk-bors bot added a commit that referenced this pull request Oct 19, 2020

Try #2249:

18f1e50

hasufell changed the title ~~Add delistPools to Pool DBLayer~~ ADP-478: Garbage collect delisted stake pools from SMASH Oct 19, 2020

hasufell self-assigned this Oct 20, 2020

cardano-foundation deleted a comment from iohk-bors bot Oct 20, 2020

jonathanknowles and others added 10 commits November 10, 2020 09:56

Rename delisted_pools table to pool_delistment.

cf7a5ad

In response to review feedback: #2277 (comment)

Make delistPools replace the set of delisted pools.

4b3c36b

We must replace the set of delisted pools, rather than augmenting it. In response to review feedback: #2277 (comment)

Update prop_putDelistedPools to check for overwriting.

08df4e1

In particular, we check that 'putDelistedPools' completely overwrites the existing set every time. In response to review feedback: #2277 (comment)

Remove prop_putDelistedPoolsPersists.

2995450

This test no longer makes sense, as delisted pools are no longer stored in the `pool_registration` table. In response to review feedback: #2277 (comment)

Adjust removePools to not remove pools from the delisted set.

b564500

In response to review feedback: #2277 (comment)

Move maintenance-actions to its own endpoint

0b8df37

Fix swagger definition

4a70362

Regenerate nix

105ec6a

hasufell force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 7579cfb to 226dea8 Compare November 10, 2020 09:44

iohk-bors bot added a commit that referenced this pull request Nov 10, 2020

Try #2249:

66b7ccd

KtorZ force-pushed the jospald/ADP-478/gc-delisted-stake-pools branch from 226dea8 to 6489807 Compare November 10, 2020 10:20

KtorZ approved these changes Nov 10, 2020

View reviewed changes

iohk-bors bot merged commit 6cdad95 into master Nov 10, 2020

iohk-bors bot deleted the jospald/ADP-478/gc-delisted-stake-pools branch November 10, 2020 11:55

hasufell mentioned this pull request Nov 10, 2020

Fix parsing of SMASHPoolId #2309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADP-478: Garbage collect delisted stake pools from SMASH #2249

ADP-478: Garbage collect delisted stake pools from SMASH #2249

hasufell commented Oct 15, 2020 •

edited

Loading

hasufell commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020

piotr-iohk commented Oct 20, 2020

hasufell commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

KtorZ left a comment

KtorZ commented Nov 10, 2020

hasufell commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

ADP-478: Garbage collect delisted stake pools from SMASH #2249

ADP-478: Garbage collect delisted stake pools from SMASH #2249

Conversation

hasufell commented Oct 15, 2020 • edited Loading

Questions / Considerations

TODO

Feature

QA

hasufell commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020

try

piotr-iohk commented Oct 20, 2020

hasufell commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

try

KtorZ left a comment

Choose a reason for hiding this comment

KtorZ commented Nov 10, 2020

hasufell commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

iohk-bors bot commented Nov 10, 2020

hasufell commented Oct 15, 2020 •

edited

Loading