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

Add pool DB operation listRetiredPools. #2016

Closed
8 tasks done
jonathanknowles opened this issue Aug 11, 2020 · 3 comments
Closed
8 tasks done

Add pool DB operation listRetiredPools. #2016

jonathanknowles opened this issue Aug 11, 2020 · 3 comments
Assignees

Comments

@jonathanknowles
Copy link
Contributor

jonathanknowles commented Aug 11, 2020

Context

As part of the work to perform garbage collection of retired pools from the database, we need to be able to determine which pools have retired.

Decision

Create a pool database operation listRetiredPools:

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

SQLite implementation

The SQLite implementation of this operation can be based on the following view, which lists all pools that have active retirement certificates associated with them:

Click to expand
CREATE VIEW active_pool_retirements AS
SELECT * FROM (
    SELECT
        pool_id,
        retirement_epoch
    FROM (
        SELECT row_number() OVER w AS r, *
        FROM (
            SELECT
                pool_id, slot, slot_internal_index, NULL as retirement_epoch
                FROM pool_registration
            UNION
            SELECT
                pool_id, slot, slot_internal_index, epoch as retirement_epoch
                FROM pool_retirement
        )
        WINDOW w AS (ORDER BY pool_id, slot desc, slot_internal_index desc)
    )
    GROUP BY pool_id
)
WHERE retirement_epoch IS NOT NULL;

Acceptance Criteria

  • The pool DB must provide a listRetiredPools operation.
  • The listRetiredPools operation must not include pools that have never had retirement certificates associated with them.
  • The listRetiredPools operation must not include pools that have had their most-recently-published retirement certificates revoked by subsequent re-registration certificates.
  • The listRetiredPools operation must not include pools with retirement epochs that are later than the specified epoch.

Development

QA

Create property tests for the model and SQLite implementations of listRetiredPools to:

  • check that they return the same results given the same inputs and initial conditions.
  • check that they do not include pools that have never had retirement certificates associated with them.
  • check that they do not include pools that have had their most-recently-published retirement certificates revoked by subsequent re-registration certificates.
  • check that they do not include pools with active retirement epochs that are later than the specified epoch.
@jonathanknowles jonathanknowles self-assigned this Aug 11, 2020
iohk-bors bot added a commit that referenced this issue Aug 17, 2020
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:<br><br>
    ```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 <jonathan.knowles@iohk.io>
iohk-bors bot added a commit that referenced this issue Aug 17, 2020
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:<br><br>
    ```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 <jonathan.knowles@iohk.io>
@piotr-iohk
Copy link
Contributor

LGTM.

@jonathanknowles this introduces new DB view active_pool_retirements. Do we need to consider any additional migration testing?

@jonathanknowles
Copy link
Contributor Author

jonathanknowles commented Aug 18, 2020

@jonathanknowles this introduces new DB view active_pool_retirements. Do we need to consider any additional migration testing?

This is a good question. I think our existing tests should be sufficient at this stage.

The view in question is created with a CREATE VIEW IF NOT EXISTS statement: https://github.com/input-output-hk/cardano-wallet/blob/75b583a14faa5bc1875f6dddd2a94a897c47080d/lib/core/src/Cardano/Pool/DB/Sqlite.hs#L489-L495

So it should be idempotent over multiple startups.

In future, if we wish to adjust this view, we can adjust the startup logic to just delete and recreate it, perhaps with a CREATE OR REPLACE VIEW statement. IMHO, this is one of the advantages of views: we can create, adjust, and delete them without having to alter existing tables with existing data.

@piotr-iohk
Copy link
Contributor

lgtm

iohk-bors bot added a commit that referenced this issue Aug 20, 2020
2053: Use ? placeholder syntax in `listRetiredPools`. r=jonathanknowles a=jonathanknowles

# Issue Number

#2016 

# Overview

This PR adjusts `listRetiredPools` to use the `?` placeholder syntax when constructing a raw SQL query.

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
iohk-bors bot added a commit that referenced this issue Aug 20, 2020
2053: Use ? placeholder syntax in `listRetiredPools`. r=jonathanknowles a=jonathanknowles

# Issue Number

#2016 

# Overview

This PR adjusts `listRetiredPools` to use the `?` placeholder syntax when constructing a raw SQL query.

Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
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

No branches or pull requests

2 participants