-
Notifications
You must be signed in to change notification settings - Fork 217
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
ADP-478: Garbage collect delisted stake pools from SMASH #2249
Conversation
865c6eb
to
0ed2886
Compare
delistPools
to Pool DBLayerdelistPools
to Pool DBLayer
b298bb2
to
c0dc034
Compare
b42eb40
to
9147481
Compare
delistPools
to Pool DBLayerdelistPools
to Pool DBLayer
a03670c
to
6327f88
Compare
6327f88
to
59a75fe
Compare
59a75fe
to
5d0c0a6
Compare
bors try |
delistPools
to Pool DBLayer
tryBuild failed: |
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)
7579cfb
to
226dea8
Compare
bors try |
tryBuild failed: |
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.
226dea8
to
6489807
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bors r+ |
1 similar comment
bors r+ |
Already running a review |
Build succeeded: |
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]>
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]>
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]>
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]>
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]>
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]>
This is the first step for garbage collecting
stake pools based on SMASH delisting.
X-JIRA-Ticket: ADP-478
Questions / Considerations
listRegisteredPools
orreadPoolProduction
. These can still consider all pools. Only the API layer will consider delisted pools and adjustApiStakePool
accordingly.removePools
at some point.TODO
Feature
delisted
column to metadata table and populate Pool DBLayer with functionsinternal_state
table and have GC thread store last sync timedelistPools
from DBLayerPOST '{ "maintenance_action": "gc_stake_pools" }' /stake-pools
endpointQA