Reorganize modules in Cardano.Wallet.DB.* #3046

HeinrichApfelmus · 2021-12-02T15:07:24Z

Issue number

Overview

Previous work in epic ADP-1043 introduced delta encodings, DBVars, and an embedding of the wallet state and its delta encodings into a database table. It's time to integrate these tools with the wallet code. To facilitate code review, the integration proceeds in a sequence of refactorings that do not change functionality and pass all unit tests.

In this step, we reorganize the modules in the Cardano.Wallet.DB.* hierarchy; the aim is to be able to quickly swap out the implementation of the mkStoreWalletsCheckpoints function by changing one import. We want to keep both the previous implementation and a subsequent new implementation around for some time.

Details

The migration function migrateManually is moved to a separate module Cardano.Wallet.DB.Sqlite.Migration. This has the beneficial side effect that this migration function cannot be accidentally be affected by refactoring in modules that are not imported by this module.
The creation of the checkpoints store, mkStoreWalletsCheckpoints, is moved to a separate module. We can swap out its implementation by importing a different module in the future.

Comments

Merge PR #2942 before this one, because this pull request is based on the branch of the former.

paweljakubas · 2021-12-03T12:11:31Z

lib/core/src/Cardano/Wallet/DB/Checkpoints.hs

+
+-- | Get the checkpoint with the largest 'SlotNo'.
+getLatest :: Checkpoints a -> (W.SlotPoint, a)
+getLatest = from . Map.lookupMax . view #checkpoints 


why not to use Either here with explicit Error. use could then also use maybeToLeft https://hackage.haskell.org/package/either-5.0.1.1/docs/src/Data.Either.Combinators.html#maybeToLeft

Hm. This error case would be a programming error, as we assume that a checkpoint for genesis is always stored, so I thought it would be fine to use a partial function here. 🤔

But I can see that if we consider Checkpoints as an abstraction independent of the database, then the error might in principle occur, e.g. we have

getLatest (apply (RollbackTo pt2) (singleton pt1 a)) = _|_

I think that the appropriate solution is actually to change singleton to only accept the genesis slot (and rename the function to fromGenesis) and to comment that RestricTo will never remove the genesis checkpoint. Then, the Checkpoints data type will satisfy the invariant that the map is always nonempty.

I will add a change to this effect.

I'm beginning to confuse myself by doing a lot rebasing, so I would like to postpone the introduction of fromGenesis to the next PR in this series.

lib/core/src/Cardano/Wallet/DB/Checkpoints.hs

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

paweljakubas · 2021-12-03T12:19:32Z

lib/core/src/Cardano/Wallet/Primitive/Types.hs

+
+-- | Magic value denoting the hash of a block that \"does not exist\".
+-- Specifically, this is the hash of \"the parent block\" of the genesis block.
+hashOfNoParent :: Hash "BlockHeader"


another approach could be to use introduce something like "parent" with Maybe...and in that case no need for magic values...it would be just Nothing

Hm. 🤔 I agree — but unfortunately, the database does serialize the parentHeaderHash in the table:

https://github.com/input-output-hk/cardano-wallet/blob/3a24c14b3208da7a6e890de1c2f3f261ab8d5b01/lib/core/src/Cardano/Wallet/DB/Sqlite/TH.hs#L210

and then we would have the magic value in the serialization. But I suppose that is an improvement over having the magic value in the type.

yes, we could probably deal with this magic value at the level of PersistField instance or so. and everywhere above we could have Maybe. just suggestion

so have magic value defined somewhere here https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/DB/Sqlite/Types.hs and toPersistValue Nothing = hashOfNoParent

I have added the hashOfNoParent constant to this module, but stopped short of changing the BlockId type. Instead, I have created two conversion functions.

lib/core/src/Cardano/Wallet/Primitive/Types.hs

paweljakubas

I like reorganizations adopted. It is seen what is new, what is old, and also that we don't affect with Checkpoints migration code at all now.

I like Checkpoint step 1 refactoring. It makes things easier to understand. To my best knowledge logic of checkpoint lifecycle is maintained (which is further evidenced by all current tests passing). I would think about introducing Maybe in parentBlockHeader and hence have Nothing rather than magic value for first block header.

Good job! Waiting for another iteration!

Also would be good if other team member would review this PR (for both double check and educational/knowledge sharing purposes)

Move * manual database migrations * `Checkpoints` type * and the creation of a `Store` for checkpoints into separate modules. The intention is that the store creation will be swapped out by a different implementation later on.

rvl · 2021-12-08T07:52:09Z

lib/core/src/Cardano/Wallet/DB/Checkpoints.hs

+Historical hack. The DB layer can't represent 'Origin' in the database,
+instead we have mapped it to 'SlotNo 0', which is wrong.


It shouldn't matter - as you write below, we don't need rollback to genesis. Only the unstable blocks can be rolled back.

rvl · 2021-12-08T07:53:41Z

lib/core/src/Cardano/Wallet/DB/Checkpoints.hs

+>>> [cardano-wallet.pools-engine:Error:1293] [2020-11-24 10:02:04.00 UTC]
+>>> Couldn't store production for given block before it conflicts with
+>>> another block. Conflicting block header is:
+>>> 5bde7e7b<-[f1b35b98-4290#2008]


To me, this looks more like rollback isn't quite working correctly in the pools db.
The #2008 means block height 2008, which is a long way from genesis.

Hm. Good point.

We did receive reports were #0 is affected:

Unexpected error following the chain:user error (restoreBlocks: given chain isn't a valid continuation. Wallet is at: 891fcd4b<-[9c9290a4-0#0] but the given chain continues starting from: 891fcd4b<-[db619f76-14#0])

Full log: https://gist.github.com/runeksvendsen/c143926249e32b22ee2d184581c9776c

Yes. But the pool db problem could be unrelated then.

HeinrichApfelmus · 2021-12-10T09:26:35Z

bors merge

iohk-bors · 2021-12-10T10:14:23Z

Build succeeded:

3048: Restrict `Checkpoints` to be constructible from genesis only r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1043 ### Overview Previous work in epic ADP-1043 introduced delta encodings, DBVars, and an embedding of the wallet state and its delta encodings into a database table. It's time to integrate these tools with the wallet code. To facilitate code review, the integration proceeds in a sequence of refactorings that do not change functionality and pass all unit tests. In this step, we implement a suggestion that came up during review where we remove the `singleton` function in favor of `fromGenesis`, making sure that the `Checkpoints` structure always contains a checkpoint for the genesis block. ### Details * The `initializeWallet` function will now throw an exception if it is incorrectly called with a checkpoint that is not at the genesis block. * Many unit tests are adapted to use the `InitialCheckpoint` generator in order to comply with the above restriction. ### Comments Merge PR #3046 before this one, because this pull request is based on the branch of the former. Co-authored-by: Heinrich Apfelmus <[email protected]>