Refactor and simplify `SeqState n k` #3073

HeinrichApfelmus · 2022-01-04T15:02:13Z

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 continue refactoring the address discovery state. Here, we refactor and simplify the SeqState n k type to use the new abstract data type Pool addr ix, which aids with BIP-44 style address discovery.

Details

The testing module PoolSpec now also provides a shrinker shrinkPool for use in the old testing module Cardano.Wallet.Primitive.AddressDiscovery.SequentialSpec.
The property tests pertaining to the address discovery aspects of SeqState are superseded by the more general unit tests in Cardano.Wallet.Address.PoolSpec.

Comments

Merge PR #Add new address Pool addr ix data type. Simplify SharedState n k. #3068 before this one, because this pull request is based on the branch of the former.

lib/core/bench/db-bench.hs

lib/core/src/Cardano/Wallet/Api/Server.hs

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

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

lib/core/src/Cardano/Wallet/Primitive/AddressDiscovery/Sequential.hs

paweljakubas · 2022-01-12T13:24:26Z

lib/core/test/unit/Cardano/Wallet/Address/PoolSpec.hs

-        it "sequence of updates preserves invariants"
-            prop_updates
+        it "sequence of updates preserves invariants" $
+            prop_updates testPool


what about property like

get to some pool state s1

clear pool -> no addresses here in the pool

update to state s1 -> state here the same as in point 1

etc? (or it is automatically tested somewhere)
also properties using removeN interleaved with ^^^

Hm, not sure. In order to test that update respects the internal invariants (prop_gap, prop_fresh), I figured that I could pick any reasonably complex sequence of updates (here addrs1 and addrs2 in prop_updates). If the code contained a bug, then it would show up in this sequence already; I'm not sure if a more systematic construction of update sequences would yield more information. 🤔 I also figured that apart from the internal invariants, the code is more or less a copy of Data.Map, and that testing that the Map is tracked correctly would be overkill. 😅

I would prefer to keep removeN for shrinking only, as it uses loadUnsafe and would have to be tested separately. In this way, we do not have to test this function: A buggy shrinker affects our ability to find the root cause of bugs, but it does not give rise to false positives or negatives.

indeed, testing Map is overkill, but maybe having roundtrip : state -> clear -> state no. Up to you. was just thinking loudly

having roundtrip : state -> clear -> state

You have convinced me — I have added a property prop_updates_order which creates a pool from a random sequence of updates and then compares this to a pool built from the Used addresses, but updated in the order of their indices. 🤓

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDiscovery/SequentialSpec.hs

paweljakubas

very nice simplification adopted. it is great that the PR resulted in -400 net loc. There are some merge conflicts to resolve before merging though.

* Use the new address pool data structure from `Cardano.Wallet.Address.Pool` * Remove `ParentContext` type * Add shrinking to testing module `Cardano.Wallet.Address.PoolSpec`

HeinrichApfelmus · 2022-01-17T13:06:20Z

bors merge

iohk-bors · 2022-01-17T13:41:42Z

Build succeeded:

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 force-pushed the HeinrichApfelmus/ADP-1043/address-pool-sequential branch 2 times, most recently from 6e41349 to 96df390 Compare January 5, 2022 12:25

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-pool-shared branch 2 times, most recently from 13ecf53 to 1323221 Compare January 5, 2022 15:49

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-pool-sequential branch 2 times, most recently from 157eaed to 12b94b3 Compare January 5, 2022 17:23

Base automatically changed from HeinrichApfelmus/ADP-1043/address-pool-shared to master January 5, 2022 21:10

HeinrichApfelmus force-pushed the HeinrichApfelmus/ADP-1043/address-pool-sequential branch 2 times, most recently from f04ecb3 to 2ebce61 Compare January 10, 2022 12:08

HeinrichApfelmus requested a review from paweljakubas January 10, 2022 21:55