Introduce `Prologue` and `Discoveries` of an address book #3056

HeinrichApfelmus · 2021-12-13T14:59:52Z

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 begin refactoring the address discovery state (the s in Wallet s). First, I would like to update our nomenclature and call this state an address book. Second, we introduce two new data families Prologue s and Discoveries s, with the intention that any address book is isomorphic to a pair of these two types, that is s ~ (Prologue s, Discoveries s). The idea is that the Prologue of the address book contains "static" information such as public keys or the address gap (default 20), while the Discoveries of the address book contains information that was gathered during on-chain address discovery.

Details

We introduce a type class AddressBookIso which provides the isomorphism s ~ (Prologue s, Discoveries s).
For this pull request, we avoid making any changes to the AddressDiscovery.* hierarchy. Instead, the isomorphisms are implemented using newly defined data families whose representation is close to the types in the database.
The implementation for the sequential states uses liftPaymentAddress to convert the payment part to and from the Address type. This is ok when convering to/from the database layer, but does not make sense for in-memory data structures. Therefore, further work in this epic will introduce changes in the AddressDiscovery.* hierarchy.

Comments

The overall goal is that, in a future PR, we can represent the wallet state along the lines of WalletState s ~ (Prologue s, Checkpoints (BlockHeader, UTxO, Discoveries s) .
Merge PR Restrict Checkpoints to be constructible from genesis only #3048 before this one, because this pull request is based on the branch of the former.

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

paweljakubas · 2021-12-20T17:51:13Z

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

+    , Typeable c
+    ) => Seq.AddressPool c key -> SeqAddressList c
+toDiscoveries pool = SeqAddressList
+    [ (a,st) | (a,st,_) <- Seq.addresses (liftPaymentAddress @n) pool ]


We do like this because SeqAddressList is directly mapped to DB, right?

if yes, then for the future reference we need to remember about some probable future developments here:

multi accountsupport - in that case liftPaymentAddress will not be enough as payment credential is not enough. We would need to also differentiate against staking credential so staking key hash (for Sequential mode) or staking script hash (for Shared mode)

we have (at least in Sequential mode) several fingerprints of address to support: the current one (aka base with key hash in payment credential), enterprise address (key hash payment credenital without staking credential) and pointer address (key hash payment with pointer stake credential)
We need (probably later) to discuss if new abstraction are able to seamlessly support them

We do like this because SeqAddressList is directly mapped to DB, right?

Yes, indeed. Will be changed again in the next steps.

[…] staking credential […]

That's a very good point. From looking at the code, I got the impression that the Sequential state only looks at the payment key hashes. In other words, whenever the state sees an address on chain (which may have a stake key hash), it will extract the payment key hash, add it to the address pool as appropriate, convert it to an enterprise address and store that address in the database. Put differently, the database stores discovered Sequential payment key hashes, but formatted as enterprise addresses.

The situation for Shared states was not entirely clear to me, but by chasing the various type classes, I also got the impression that only the payment key hash is subject to discovery. In other words, if an address appears on the chain, and the stake credential of this address does not match the delegation template, but the payment credential does match the payment template, then it will still be discovered and added to the database. Makes sense: If you own the payment credential, you can always reassign the delegation part by making a transaction; the access control on the payment credential partially overrides the access control on the delegation part. I suppose we will need to think about this corner case again. 🤔

In any case, for the time being, this PR only attempts to do what the code did before, no changes in functionality intended. 😅

so for shared wallets we store payment credential only which is script hash. shared wallet's credential and sequential wallet credential happen to have the same byte imprint as the same hashes (blake224) in both cases is used.

What is important here is that maybe to avoid complications and choose simplicity it would be better to store both credentials, whatever they are represented (keyhash, scripthash or three numbers for pointer addresses). It would allow support of any address type and also seamlessly support multiaccount (on the level of discovery)

I feel we will need to revisit this soon or later (maybe even during shared wallet work soon). I also see challenges (db migration issues....)

I feel we will need to revisit this soon or later (maybe even during shared wallet work soon). I also see challenges (db migration issues....)

Agreed. I suggest that we leave things as they are for now and revisit this soon during the shared wallet work.

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

paweljakubas · 2021-12-20T18:02:22Z

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

    => W.WalletId
    -> SqlPersistT IO [(W.Slot, W.Wallet s)]
 selectAllCheckpoints wid = do
    cps <- fmap entityVal <$> selectList
        [ CheckpointWalletId ==. wid ]
        [ Desc CheckpointSlot ]
-    fmap catMaybes $ forM cps $ \cp -> do
+    -- FIXME LATER during ADP-1043: Presence of these tables?


would be good to have check on if the tables exist and only after they are proceed 👍

Good point. I have added a commit where a new error type ErrBadFormat is introduced, which I intend to eventually cover all errors related to malformed database files. As a first step, it absorbs the fromJust case here.

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

paweljakubas · 2021-12-20T18:06:34Z

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

-            <$> st
+        discoveries <- loadDiscoveries wid (checkpointSlot cp)
+        let st = withIso addressIso $ \_ from -> from (prologue, discoveries)
+        pure $ let c = checkpointFromEntity @s cp utxo st in (getPoint c, c)


would be in favor of extracting let in the above line. just for the sake of clarity

Added in the additional commit.

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

lib/core/test/unit/Cardano/WalletSpec.hs

paweljakubas

great PR! very fond of introducing of decoupling between nonchanging and changing parts of the state. And treat them explicitely as separate.

In order to avoid making changes to the `AddressDiscovery.*` modules for the time being, we decompose the address state by means of two data families and an isomorphism. * The `Prologue` captures off-chain information such as public keys or address gaps. This information is not associated with chain checkpoints. * The `Discoveries` capture on-chain information such as discovered addresses in the `AddressPool`. This information has to be associated with chain checkpoints.

To be thrown when the database file is in a bad format (missing tables, malformatted entries, other forms of corruption, …)

HeinrichApfelmus · 2021-12-22T12:41:22Z

bors merge

iohk-bors · 2021-12-22T13:13:52Z

Build succeeded:

3068: Add new address `Pool addr ix` data type. Simplify `SharedState n k`. r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1308 ### 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 continue refactoring the address discovery state. * First, we implement a new abstract data type, `Pool addr ix`, which aids with BIP-44 style address discovery. Its main benefit is that it is polymorphic in the address type and thus focuses on the essence of address discovery with an address gap while avoiding the plethora of type classes related to specific address formats. * Then, we use this data type to simplify the implementation of the address discovery state for mult-signature wallets, `SharedState n k`. ### Details * `Pool` is an abstract data type, its constructor is not to be exported. Its internal invariants are collected in `prop_consistent`. To bypass the invariants, use `loadUnsafe`; the database layer is allowed to do that. * Some of the unit tests pertaining to the address discovery aspects `SharedState` are partially superseded by the more general unit tests in `Cardano.Wallet.Address.PoolSpec`. ### Comments * The `SeqState n k` states will be refactored in terms of the new `Pool addr ix` data type next. * Merge PR #3056 before this one, because this pull request is based on the branch of the former. Co-authored-by: Heinrich Apfelmus <[email protected]>

3099: Add `WalletState`, a pure model for the entire wallet r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1375 ### 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 introduce a data type `WalletState` which represents the entire wallet state — not just the most recent checkpoint, but _all_ checkpoints. ``` data WalletState s = WalletState { prologue :: Prologue s , checkpoints :: Checkpoints (BlockHeader, UTxO, Discoveries s) } ``` The states for the different wallets currently stored in the `walletsVar` DBVar. Eventually, the data type will become the purely functional in-memory representation of all the data associated with a wallet (though perhaps with a different name). The `DBLayer` type will eventually be replaced by a `Store` for values of this type. The introduction of this type has become possible thanks to the previous separation of the address discovery state `s` into `Prologue s` and `Discoveries s` (PRs #3056, #3068, #3073). ### Details * As the queries in the `DBLayer` will more and more become queries on the in-memory cache `walletsVar` instead of queries on the database table, I have begun to add unit tests for the database tables. Here, I have added a property test for `loadPrologue` and `insertPrologue`. Eventually, these separate tests will become a single generic test for a `Store` of `Table`. ### Comments * One of the next steps will be to replace `UTxO` in the above type by its delta encoding `DeltaUTxO`. This will reduce the memory footprint of the in-memory representation. * The `Cardano.Wallet.DB.Model` module actually implements a pure model for the entire wallet state, including TxHistory etc. When extending the type, we can scavenge from there. Co-authored-by: Heinrich Apfelmus <[email protected]>

3099: Add `WalletState`, a pure model for the entire wallet r=HeinrichApfelmus a=HeinrichApfelmus ### Issue number ADP-1375 ### 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 introduce a data type `WalletState` which represents the entire wallet state — not just the most recent checkpoint, but _all_ checkpoints. ``` data WalletState s = WalletState { prologue :: Prologue s , checkpoints :: Checkpoints (BlockHeader, UTxO, Discoveries s) } ``` The states for the different wallets currently stored in the `walletsVar` DBVar. Eventually, the data type will become the purely functional in-memory representation of all the data associated with a wallet (though perhaps with a different name). The `DBLayer` type will eventually be replaced by a `Store` for values of this type. The introduction of this type has become possible thanks to the previous separation of the address discovery state `s` into `Prologue s` and `Discoveries s` (PRs #3056, #3068, #3073). ### Details * As the queries in the `DBLayer` will more and more become queries on the in-memory cache `walletsVar` instead of queries on the database table, I have begun to add unit tests for the database tables. Here, I have added a property test for `loadPrologue` and `insertPrologue`. Eventually, these separate tests will become a single generic test for a `Store` of `Table`. ### Comments * One of the next steps will be to replace `UTxO` in the above type by its delta encoding `DeltaUTxO`. This will reduce the memory footprint of the in-memory representation. * The `Cardano.Wallet.DB.Model` module actually implements a pure model for the entire wallet state, including TxHistory etc. When extending the type, we can scavenge from there. Co-authored-by: Heinrich Apfelmus <[email protected]> Co-authored-by: IOHK <[email protected]>

HeinrichApfelmus changed the base branch from master to HeinrichApfelmus/ADP-1043/fromGenesis December 13, 2021 15:00

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-book-iso branch 2 times, most recently from a01336e to dfc7b0c Compare December 13, 2021 16:19

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/fromGenesis branch from e2d8e6c to c54becd Compare December 14, 2021 12:12

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-book-iso branch 6 times, most recently from da72335 to b389528 Compare December 15, 2021 10:27

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/fromGenesis branch from 163c67a to d68b1e5 Compare December 16, 2021 13:29

Base automatically changed from HeinrichApfelmus/ADP-1043/fromGenesis to master December 16, 2021 17:04

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-book-iso branch from b389528 to 6d60731 Compare December 16, 2021 17:19

HeinrichApfelmus requested a review from paweljakubas December 20, 2021 11:56

HeinrichApfelmus mentioned this pull request Dec 20, 2021

Add new address Pool addr ix data type. Simplify SharedState n k. #3068

Merged