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

Use SlotNo and TimeInterpreter #1901

Merged
merged 11 commits into from
Jul 17, 2020
Merged

Conversation

Anviking
Copy link
Collaborator

@Anviking Anviking commented Jul 13, 2020

Issue Number

ADP-356 #1868 #1869

Overview

  • Add TimeInterpreter, necessary queries, and a way to run them
    Add tests comparing them with old implementation

  • Fundamentally change wallet to use SlotNo instead of SlotId

    • Patch together cardano-wallet-jormungandr to still work, despite the
      binary using SlotId.
  • Remove redundant byron code

  • Sync progress calculation compares times now instead of slots

  • Rename the slot tables to slot_no to force automatic migration, preventing catastrophic re-interpretation of old data

Comments

@Anviking Anviking self-assigned this Jul 13, 2020
@Anviking Anviking changed the base branch from master to anviking/1868/move-slotting July 13, 2020 14:59
-- We cannot manually specify when the fetching happens.
--
-- This may or may not be what we actually want.
type TimeInterpreter m = forall a. Qry a -> m a
Copy link
Collaborator Author

@Anviking Anviking Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ We may now have a TimeInterpreter IO that sometimes takes a long time to run a conversion query (when it needs to refetch the summary over LSQ).

Not sure if we should change…

The re-fetching should happen on very rare occasions though…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have the network layer query the Interpreter on a schedule, and stash it in a mutable variable. So that all queries can be pure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that all queries can be pure

I presume it would still be in IO (to access the var), so no change to the interface.

In that case I'd be concerned with what callers would see if there is nothing in the var. There are many call-sites, and I think blocking until it is available is the simplest thing to. Which is somewhat like this.

But preemptively refreshing before a conversion fails, sounds sane to do… optimally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see. I was thinking that the initial Interpreter will be one constructed from genesis parameters.
It will get updated once the node has synced far enough. (Question - how far is enough?)
This would prevent the wallet from being unresponsive while the node is syncing.

@rvl rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jul 14, 2020
@Anviking Anviking force-pushed the anviking/1868/move-slotting branch from b2e34a7 to 1be1ac1 Compare July 14, 2020 09:50
Base automatically changed from anviking/1868/move-slotting to master July 14, 2020 11:29
@Anviking Anviking force-pushed the anviking/1868/time-interpreter branch from 315b875 to 249af6f Compare July 14, 2020 17:10
@@ -90,7 +92,7 @@ TxMeta
txMetaWalletId W.WalletId sql=wallet_id
txMetaStatus W.TxStatus sql=status
txMetaDirection W.Direction sql=direction
txMetaSlot W.SlotId sql=slot
txMetaSlot SlotNo sql=slot
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we may need manual migration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't fret over DB migration if it's too hard. We can start from a fresh state directory if necessary.

Copy link
Collaborator Author

@Anviking Anviking Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I fear/wonder if users would silently get corrupted data, as the meaning of the slot table has changed, but not the name.

Is there a good way to test / detect this?

Or I guess testing manually using CLI works. I tried using the Daedalus mainnet installation, and replacing the wallet and node manually, but I think too many other things have changed for it to work.

Copy link
Contributor

@rvl rvl Jul 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type of the column changes, I think persistent auto migrations will either throw an exception or helpfully delete the table.

Let's decide next week whether to automatically migrate or make a fresh state directory.

@Anviking Anviking added IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG and removed ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG labels Jul 14, 2020
@Anviking Anviking force-pushed the anviking/1868/time-interpreter branch from 630c90b to c2d6707 Compare July 15, 2020 14:49
@Anviking Anviking force-pushed the anviking/1868/time-interpreter branch from 5e4b66a to 8d0a0af Compare July 16, 2020 12:57
@rvl
Copy link
Contributor

rvl commented Jul 16, 2020

What's the benefit of wrapping the provided Interpreter in our own type? Can we use the bare Interpreter?

@Anviking
Copy link
Collaborator Author

What's the benefit of wrapping the provided Interpreter in our own type?

The original interpreter only works with RelativeTime, which is relative to genesis.

The wrapper carries the genesis start date along with it.

== Add TimeInterpreter, necessary queries, and a way to run them
Add tests comparing them with old implementation

== Fundamentally change wallet to use SlotNo instead of SlotId
- Patch together cardano-wallet-jormungandr to still work, despite the
binary using SlotId.

== Remove redundant byron code

== Sync progress calculation compares times now instead of slots
@Anviking Anviking force-pushed the anviking/1868/time-interpreter branch from 8d0a0af to 353f4b6 Compare July 16, 2020 14:12
@Anviking
Copy link
Collaborator Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 16, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 16, 2020

try

Build failed

@Anviking
Copy link
Collaborator Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 16, 2020
@Anviking
Copy link
Collaborator Author

I am seeing rare failures of this sort. I am investigating

  test/unit/Cardano/Pool/DB/Properties.hs:348:17:
  1) Cardano.Pool.DB.Sqlite.Sqlite, Stake Pool properties, readPoolProduction for a given epoch should always give slots from given epoch
       Falsifiable (after 10 tests and 1 shrink):
         StakePoolsFixture {poolSlots = [(PoolId {getPoolId = "w\EOTe\227Aa\169\r=\fK%v>C\178\r/\ETX\v\FS\131>\ACK\225\aG$u\a\v>"},BlockHeader {slotNo = SlotNo {unSlotNo = 10}, blockHeight = Quantity {getQuantity = 0}, headerHash = Hash {getHash = "00000000000000000000000000000001"}, parentHeaderHash = Hash {getHash = "00000000000000000000000000000000"}}),(PoolId {getPoolId = "\138P0PcK\141\161.\a\DC1\DC3L\ESC\a\SOT\FS\US\245[\209&Q\CAN\DLEWU\249\DC1\165?"},BlockHeader {slotNo = SlotNo {unSlotNo = 0}, blockHeight = Quantity {getQuantity = 0}, headerHash = Hash {getHash = "00000000000000000000000000000001"}, parentHeaderHash = Hash {getHash = "00000000000000000000000000000000"}}),(PoolId {getPoolId = "w\EOTe\227Aa\169\r=\fK%v>C\178\r/\ETX\v\FS\131>\ACK\225\aG$u\a\v>"},BlockHeader {slotNo = SlotNo {unSlotNo = 8}, blockHeight = Quantity {getQuantity = 0}, headerHash = Hash {getHash = "00000000000000000000000000000001"}, parentHeaderHash = Hash {getHash = "00000000000000000000000000000000"}})], rollbackSlots = [SlotNo {unSlotNo = 10},SlotNo {unSlotNo = 0},SlotNo {unSlotNo = 8}]}
       expected: fromList [SlotNo {unSlotNo = 0},SlotNo {unSlotNo = 8}]
        but got: fromList [SlotNo {unSlotNo = 10}]

It would group slots like:

Epoch 1:
  SlotNo 0
  SlotNo 8
Epoch 0:
  SlotNo 11

when it zipped the grouped slots together with the epochs.

I'm not sure how it worked previously and stopped working now.

I also added a somewhat ugly shrinker for the stake pool fixtures.
This reverts commit 9eb843b.

That commit broke a unit test failure for a manual migration. I am not
sure if the test could have been removed without consequences.

But weirdly, migration seems to work smoothly without it:

$ cardano-wallet-shelley-2020-07-06 serve --node-socket socket --testnet shelley_testnet-genesis.json --database wallet-db
$ # Create a wallet
$ cardano-wallet-shelley transaction list <WALLET ID>

$ cardano-wallet-shelley serve --node-socket socket --testnet shelley_testnet-genesis.json --database wallet-db
$ cardano-wallet-shelley transaction list <WALLET ID>

$ # The correct slots are still shown.

Looking in the DBs, it seems like wallet does detect that the schema has
changed, and re-syncs from genesis.
@Anviking
Copy link
Collaborator Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 16, 2020
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 16, 2020

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work - this is a really big PR.
I have checked Cardano.Wallet.Primitive and Cardano.Wallet.Shelley.
Is there anything else I should pay attention to?

lib/core/src/Cardano/Wallet/Primitive/Slotting.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Types.hs Outdated Show resolved Hide resolved

-- * Old functions
-- * Legacy api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these all be removed in a future PR, or moved to a tests-only module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we should remove them completely, and re-write the tests to use the new ones.

Now we are testing the old ones, and that the new ones are equivalent to the old ones (Generators aren't perfect though).

defaultTTL epochLength slot =
(toSlotNo epochLength slot) + 7200
defaultTTL :: SlotNo -> SlotNo
defaultTTL slot = slot + 7200
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. :-)

@rvl
Copy link
Contributor

rvl commented Jul 17, 2020

With the database migration issue, we can't re-use the slot columns if the meaning of their values has changed. So we have to do something about this situation.

We could either:

  1. implement some "manual migration" code to restore all wallets from genesis if loading a DB from before this change.
  2. say that the DB is incompatible with previous versions, and a fresh state directory must be used.

In the case of 2, the DB migration test can be disabled, because there is no migration to test. I like option 2, because there are other user stories such as transaction TTL that also need major DB schema changes.

@Anviking
Copy link
Collaborator Author

Anviking commented Jul 17, 2020

Migrating from 2020-07-06 works. Migrating from the current flight candidate to this PR doesn't:

Skärmavbild 2020-07-17 kl  12 47 59

But in the real world, users would be upgrading from an older Daedalus Mainnet release, which might trigger automatic migration.

In the case of 2, the DB migration test can be disabled, because there is no migration to test. I like option 2, because there are other user stories such as transaction TTL that also need major DB schema changes.

✔️

Regardless, I think thinking about this later is fine, as you suggested in another comment.

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking

# Issue Number

ADP-356 #1868 #1869 


# Overview

- [x] Add TimeInterpreter, necessary queries, and a way to run them
Add tests comparing them with old implementation

- [x] Fundamentally change wallet to use SlotNo instead of SlotId
    - Patch together cardano-wallet-jormungandr to still work, despite the
binary using SlotId.

- [x] Remove redundant byron code

- [x] Sync progress calculation compares times now instead of slots

- [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data

# Comments

- Depends on #1890 

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@Anviking Anviking force-pushed the anviking/1868/time-interpreter branch from 58bff35 to 1014550 Compare July 17, 2020 11:49
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

Canceled

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking

# Issue Number

ADP-356 #1868 #1869 


# Overview

- [x] Add TimeInterpreter, necessary queries, and a way to run them
Add tests comparing them with old implementation

- [x] Fundamentally change wallet to use SlotNo instead of SlotId
    - Patch together cardano-wallet-jormungandr to still work, despite the
binary using SlotId.

- [x] Remove redundant byron code

- [x] Sync progress calculation compares times now instead of slots

- [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data

# Comments

- Depends on #1890 

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

Build failed

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking

# Issue Number

ADP-356 #1868 #1869 


# Overview

- [x] Add TimeInterpreter, necessary queries, and a way to run them
Add tests comparing them with old implementation

- [x] Fundamentally change wallet to use SlotNo instead of SlotId
    - Patch together cardano-wallet-jormungandr to still work, despite the
binary using SlotId.

- [x] Remove redundant byron code

- [x] Sync progress calculation compares times now instead of slots

- [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data

# Comments

- Depends on #1890 

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

Build failed

  test/unit/Cardano/Wallet/Primitive/SlottingSpec.hs:69:13: 
  1) Cardano.Wallet.Primitive.Slotting.slotting, runQuery NEW singleEraInterpreter == OLD . fromFlatSlot, slotRangeFromTimeRange and slotRangeFromTimeRange'
       uncaught exception: ErrorCall
       fromFlatSlot: The specified flat slot number (7469933546) is higher than the maximum flat slot number (6442450943) for the specified epoch length (3).
       CallStack (from HasCallStack):
         error, called at src/Cardano/Wallet/Primitive/Slotting.hs:331:9 in cardano-wallet-core-2020.7.6-FPg0XPMd6uj7UsprjiHPOH:Cardano.Wallet.Primitive.Slotting
       (after 912301 tests and 3 shrinks)
         GenesisParameters {getGenesisBlockHash = Hash {getHash = "Genesis Hash"}, getGenesisBlockDate = StartTime 1865-01-03 04:58:42 UTC, getSlotLength = SlotLength {unSlotLength = 0.145323864972s}, getEpochLength = EpochLength {unEpochLength = 3}, getEpochStability = Quantity {getQuantity = 0}, getActiveSlotCoefficient = ActiveSlotCoefficient {unActiveSlotCoefficient = 1.0}}
         Range {inclusiveLowerBound = Nothing, inclusiveUpperBound = Just 1899-05-29 13:18:56 UTC}
         Exception thrown while showing test case:
           fromFlatSlot: The specified flat slot number (7469933546) is higher than the maximum flat slot number (6442450943) for the specified epoch length (3).
           CallStack (from HasCallStack):
             error, called at src/Cardano/Wallet/Primitive/Slotting.hs:331:9 in cardano-wallet-core-2020.7.6-FPg0XPMd6uj7UsprjiHPOH:Cardano.Wallet.Primitive.Slotting
         

  To rerun use: --match "/Cardano.Wallet.Primitive.Slotting/slotting/runQuery NEW singleEraInterpreter == OLD . fromFlatSlot/slotRangeFromTimeRange and slotRangeFromTimeRange'/"

Randomized with seed 212063873

@Anviking
Copy link
Collaborator Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
1901: Use SlotNo and TimeInterpreter r=Anviking a=Anviking

# Issue Number

ADP-356 #1868 #1869 


# Overview

- [x] Add TimeInterpreter, necessary queries, and a way to run them
Add tests comparing them with old implementation

- [x] Fundamentally change wallet to use SlotNo instead of SlotId
    - Patch together cardano-wallet-jormungandr to still work, despite the
binary using SlotId.

- [x] Remove redundant byron code

- [x] Sync progress calculation compares times now instead of slots

- [x] Rename the `slot` tables to `slot_no` to force automatic migration, preventing catastrophic re-interpretation of old data

# Comments

- Depends on #1890 

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

Build failed

error: unable to fork: Cannot allocate memory

@Anviking
Copy link
Collaborator Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit cfdfc33 into master Jul 17, 2020
@iohk-bors iohk-bors bot deleted the anviking/1868/time-interpreter branch July 17, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMPROVEMENT Mark a PR as an improvement, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants