Add pool DB operation `listRetiredPools`. #2024

jonathanknowles · 2020-08-11T09:26:08Z

Issue Number

#2016

Overview

This PR:

Adds a listRetiredPools operation to the pool DB layer:

listRetiredPools
    :: EpochNo
    -> stm [PoolId]
    -- ^ List all pools with an active retirement epoch that is
    -- earlier than or equal to the specified epoch.

Provides the following implementations of listRetiredPools.
- Pool.DB.MVar
- Pool.DB.SQLite.

The SQLite implementation is based on an SQL view (see definition) which determines the currently-active set of retirement certificates.

Click to view sample data

Property Testing

This PR also adds property tests to check that:

listRetiredPools does not return:
- pools that have never had retirement certificates associated with them.
- pools that have had their most-recently-published retirement certificates revoked by subsequent re-registration certificates.
- pools with effective retirement epochs that are later than the specified epoch.
listRetiredPools returns the correct set of pools in the following contexts:
- where there are multiple pools.
- where there are multiple certificates per pool
  (registrations, retirements, re-registrations, re-retirements).
- where the publications of certificates affecting different pools are interleaved together in time.

Mutation Testing

To increase confidence that the activePoolRetirements view is correctly defined, I have manually performed the following mutations and verified that the prop_listRetiredPools_multiplePools_multipleCerts property fails appropriately.

Mutation 1

-    WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc)
+    WINDOW w AS (ORDER BY pool_id, slot     , slot_internal_index desc)
 )
 GROUP BY pool_id

Mutation 2

-    WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc)
+    WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index     )
 )
 GROUP BY pool_id

Mutation 3

-    WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc)
+    WINDOW w AS (ORDER BY pool_id, slot     , slot_internal_index     )
 )
 GROUP BY pool_id

Mutation 4

-    WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc)
+    WINDOW w AS (ORDER BY pool_id, slot desc,                         )
 )
 GROUP BY pool_id

KtorZ · 2020-08-12T12:06:10Z

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

+            let safeCast (Single poolId, Single retirementEpoch) =
+                    PoolRetirementCertificate
+                        <$> fromPersistValue poolId
+                        <*> fromPersistValue retirementEpoch


Hmmm. Why is the unsafe query needed here? We should stick with the persistent DSL as much as possible and only resort to unsafe queries when the DSL is too limiting or becomes a real performance impediment.

@KtorZ wrote:

Why is the unsafe query needed here?

When determining the current set of effective retirements, we need to combine results from two different tables:

pool_registrations

pool_retirements

This is because registration and retirement certificates can cancel each other out.

The only way to find out whether a retirement certificate is still in effect is to verify that there are no other retirement certificates or registration certificates that cancel it out.

we should only resort to unsafe queries when the DSL is too limiting

I do agree with this.

However, the implementation in this PR determines the set of currently-active retirement certificates with a single query that uses:

the SQL UNION function;

the SQL row_number window function.

As far as I know, persistent supports neither of the above functions (at least out of the box).

Furthermore, defining this query in terms of an SQL view can help us if we need to debug a real pool database. By opening the database and fetching the active_pool_retirements view, we can immediately see which retirement certificates the wallet currently considers to be active.

Here's the active_pool_retirements view for mainnet:

To implement listRetiredPools, we just need to select all rows from this view with a retirement_epoch that is earlier than or equal to the given epoch argument.

Extend `prop_pool{Registration,Retirement}` to call `listRetiredPools`.

Extend `prop_rollbackRetirement` to call `listRetiredPools`.

Extend `prop_readPoolLifeCycleStatus` to call `listRetiredPools`.

Use different serialization orders when serializing certificate sequences from multiple pools in property `prop_listRetiredPools_multiplePools_multipleCerts`. This gives us extra confidence that the `activeRetirementCertificates` view is handling the `(slot, slot_internal_index)` order correctly.

jonathanknowles · 2020-08-13T10:26:18Z

bors try

iohk-bors · 2020-08-13T11:23:37Z

try

Build succeeded

KtorZ · 2020-08-17T07:09:37Z

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

+        assertWith "entriesIn == entriesOut"
+            $ entriesIn == entriesOut
+        assertWith "no pools are marked to retire"
+            $ null poolsMarkedToRetire


KtorZ · 2020-08-17T07:10:45Z

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

+        assertWith "all pools are marked to retire"
+            $ (==)
+                (Set.fromList $ snd <$> entriesIn)
+                (Set.fromList poolsMarkedToRetire)


KtorZ · 2020-08-17T07:12:01Z

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

            mRetrievedCertificatePublication
            mExpectedCertificatePublication
+        assertWith "pool is not marked to retire" $
+            null poolsMarkedToRetire


KtorZ · 2020-08-17T07:37:42Z

lib/core/test/unit/Cardano/Pool/DB/Properties.hs

+        lifeCycleStatuses <- run $ atomically $ do
+            mapM readPoolLifeCycleStatus allPoolIds
+        let poolsMarkedToRetire = catMaybes $
+                getPoolRetirementCertificate <$> lifeCycleStatuses


I'd be better not to rely on lifeCycleStatuses to determine which pools are supposed to be retired. The risk is to have a similar logic bug in both lifeCycleStatuses and listRetiredPools that remain uncaught, especially if the same approach is used behind the scene.

I'd be better not to rely on lifeCycleStatuses to determine which pools are supposed to be retired. The risk is to have a similar logic bug in both lifeCycleStatuses and listRetiredPools that remain uncaught, especially if the same approach is used behind the scene.

I definitely agree, this is indeed a risk.

To counteract this risk:

We do have a suite of tests specifically aimed at determinePoolLifeCycleStatus:

prop_determinePoolLifeCycleStatus_orderCorrect

prop_determinePoolLifeCycleStatus_neverRegistered

prop_determinePoolLifeCycleStatus_differentPools

The definition of listRetiredPools uses a completely different approach in the form of a handwritten query. In the property test for this query, we check it against the determinePoolLifeCycleStatus function, which we consider to be the source of truth.

jonathanknowles · 2020-08-17T07:49:21Z

bors r+

2024: Add pool DB operation `listRetiredPools`. r=jonathanknowles a=jonathanknowles # Issue Number #2016 # Overview This PR: - [x] Adds a `listRetiredPools` operation to the pool DB layer: ```hs listRetiredPools :: EpochNo -> stm [PoolId] -- ^ List all pools with an active retirement epoch that is -- earlier than or equal to the specified epoch. ```` - [x] Provides the following implementations of `listRetiredPools`. - [x] `Pool.DB.MVar` - [x] `Pool.DB.SQLite`. The SQLite implementation is based on an **SQL view** ([see definition](https://github.com/input-output-hk/cardano-wallet/pull/2024/files#diff-08afa2852f50fa31f5757942d004ce8aR513)) which determines the currently-active set of retirement certificates. <details><summary>Click to view sample data</summary> ![active_pool_retirements](https://user-images.githubusercontent.com/206319/90096831-13b1a280-dd67-11ea-9612-fd1c4a825fb6.png) </details> # Property Testing This PR also adds property tests to check that: - [x] `listRetiredPools` **does not** return: - [x] pools that have never had retirement certificates associated with them. - [x] pools that have had their most-recently-published retirement certificates revoked by subsequent re-registration certificates. - [x] pools with effective retirement epochs that are later than the specified epoch. - [x] `listRetiredPools` returns the correct set of pools in the following contexts: - [x] where there are **multiple pools**. - [x] where there are **multiple certificates per pool** (registrations, retirements, re-registrations, re-retirements). - [x] where the publications of certificates affecting different pools are **interleaved together in time**. # Mutation Testing To increase confidence that the `activePoolRetirements` view is correctly defined, I have manually performed the following mutations and verified that the `prop_listRetiredPools_multiplePools_multipleCerts` property fails appropriately. ### Mutation 1 ```patch - WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc) + WINDOW w AS (ORDER BY pool_id, slot , slot_internal_index desc) ) GROUP BY pool_id ``` ### Mutation 2 ```patch - WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc) + WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index ) ) GROUP BY pool_id ``` ### Mutation 3 ```patch - WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc) + WINDOW w AS (ORDER BY pool_id, slot , slot_internal_index ) ) GROUP BY pool_id ``` ### Mutation 4 ```patch - WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc) + WINDOW w AS (ORDER BY pool_id, slot desc, ) ) GROUP BY pool_id ``` Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2020-08-17T08:39:41Z

Build failed

buildkite/cardano-wallet

jonathanknowles · 2020-08-17T08:41:24Z

bors retry

iohk-bors · 2020-08-17T09:34:52Z

Build succeeded

2047: Add pool DB operation `removeRetiredPools`. r=jonathanknowles a=jonathanknowles # Issue Number #2018 # Overview Building on the work added in PRs #2024 and #2038, this PR: - [x] Adds a `removeRetiredPools` operation to the pool DB layer: ```hs removeRetiredPools :: EpochNo -> stm [PoolRetirementCertificate] -- ^ Remove all pools with an active retirement epoch that is earlier -- than or equal to the specified epoch. -- -- Returns the retirement certificates of the pools that were removed. ``` - [x] Writes a bracketed `MsgRemovingRetiredPools` message to the log whenever the `removeRetiredPools` operation is called: ``` Looking for pools that retired in or before epoch 1000. Removing the following retired pools: Pool 4355a46b with retirement epoch 999 Pool 53c234e5 with retirement epoch 999 Pool 121cfccd with retirement epoch 1000 Pool 917df332 with retirement epoch 1000 Finished removing retired pools. ``` # Further Work Before garbage collection can actually happen, the wallet needs to call `removeRetiredPools` at appropriate intervals. This work is deferred to a future PR. See issue #2019. Co-authored-by: Jonathan Knowles <[email protected]>

jonathanknowles added DO NOT MERGE IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG labels Aug 11, 2020

jonathanknowles self-assigned this Aug 11, 2020

jonathanknowles force-pushed the jonathanknowles/list-retired-pools branch 2 times, most recently from be4151a to ee71e8a Compare August 11, 2020 10:14

jonathanknowles added this to the (ADP-376) Garbage collection of retired pools from the DB milestone Aug 11, 2020

jonathanknowles force-pushed the jonathanknowles/list-retired-pools branch 10 times, most recently from cd555c8 to fcbc5f2 Compare August 12, 2020 11:38

jonathanknowles removed the DO NOT MERGE label Aug 12, 2020

jonathanknowles changed the title ~~WIP: Add pool DB operation listRetiredPools.~~ Add pool DB operation listRetiredPools. Aug 12, 2020

jonathanknowles marked this pull request as ready for review August 12, 2020 11:44

KtorZ reviewed Aug 12, 2020

View reviewed changes

jonathanknowles requested a review from KtorZ August 12, 2020 12:39

jonathanknowles force-pushed the jonathanknowles/list-retired-pools branch from fcbc5f2 to 999db87 Compare August 13, 2020 05:09

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Aug 13, 2020

Try #2024:

a06fc1b

This comment has been minimized.

Sign in to view

jonathanknowles added 4 commits August 13, 2020 07:25

Add dependency on string-qq package.

98087ca

Add pool database view active_pool_retirements.

1d9b5b5

Add skeleton listRetiredPools pool DB operation.

9554eba

Provide model implementation of listRetiredPools operation.

7910adf

jonathanknowles added 5 commits August 13, 2020 07:25

Extend prop_pool{Registration,Retirement}.

7f4745f

Extend `prop_pool{Registration,Retirement}` to call `listRetiredPools`.

Extend prop_rollbackRetirement.

e539aa2

Extend `prop_rollbackRetirement` to call `listRetiredPools`.

Extend prop_readPoolLifeCycleStatus.

bf697df

Extend `prop_readPoolLifeCycleStatus` to call `listRetiredPools`.

Simplify property prop_readPoolLifeCycleStatus.

3c4c614

Add property prop_listRetiredPools_multiplePools_multipleCerts.

c61f7df

jonathanknowles force-pushed the jonathanknowles/list-retired-pools branch from 15814ae to 65b143d Compare August 13, 2020 07:28

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Aug 13, 2020

Try #2024:

c0c4b65

This comment has been minimized.

Sign in to view

iohk-bors bot added a commit that referenced this pull request Aug 13, 2020

Try #2024:

371b252

This comment has been minimized.

Sign in to view

jonathanknowles force-pushed the jonathanknowles/list-retired-pools branch from 65b143d to c59e8e8 Compare August 13, 2020 10:26

iohk-bors bot added a commit that referenced this pull request Aug 13, 2020

Try #2024:

24ba541

jonathanknowles requested a review from rvl August 13, 2020 11:31

KtorZ approved these changes Aug 17, 2020

View reviewed changes

iohk-bors bot merged commit 00b518c into master Aug 17, 2020

iohk-bors bot deleted the jonathanknowles/list-retired-pools branch August 17, 2020 09:34

jonathanknowles mentioned this pull request Aug 18, 2020

Add pool DB operation removeRetiredPools. #2047

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pool DB operation `listRetiredPools`. #2024

Add pool DB operation `listRetiredPools`. #2024

jonathanknowles commented Aug 11, 2020 •

edited

Loading

KtorZ Aug 12, 2020

jonathanknowles Aug 12, 2020 •

edited

Loading

jonathanknowles Aug 12, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jonathanknowles commented Aug 13, 2020

iohk-bors bot commented Aug 13, 2020

KtorZ Aug 17, 2020

KtorZ Aug 17, 2020

KtorZ Aug 17, 2020

KtorZ Aug 17, 2020

jonathanknowles Aug 17, 2020 •

edited

Loading

jonathanknowles commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

jonathanknowles commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

Add pool DB operation listRetiredPools. #2024

Add pool DB operation listRetiredPools. #2024

Conversation

jonathanknowles commented Aug 11, 2020 • edited Loading

Issue Number

Overview

Property Testing

Mutation Testing

Mutation 1

Mutation 2

Mutation 3

Mutation 4

KtorZ Aug 12, 2020

Choose a reason for hiding this comment

jonathanknowles Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

jonathanknowles Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jonathanknowles commented Aug 13, 2020

iohk-bors bot commented Aug 13, 2020

try

Build succeeded

KtorZ Aug 17, 2020

Choose a reason for hiding this comment

KtorZ Aug 17, 2020

Choose a reason for hiding this comment

KtorZ Aug 17, 2020

Choose a reason for hiding this comment

KtorZ Aug 17, 2020

Choose a reason for hiding this comment

jonathanknowles Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

jonathanknowles commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

Build failed

jonathanknowles commented Aug 17, 2020

iohk-bors bot commented Aug 17, 2020

Build succeeded

Add pool DB operation `listRetiredPools`. #2024

Add pool DB operation `listRetiredPools`. #2024

jonathanknowles commented Aug 11, 2020 •

edited

Loading

jonathanknowles Aug 12, 2020 •

edited

Loading

jonathanknowles Aug 12, 2020 •

edited

Loading

jonathanknowles Aug 17, 2020 •

edited

Loading