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

Extend the current DB layer with an SQLite interface #154

Closed
12 tasks done
akegalj opened this issue Apr 4, 2019 · 3 comments
Closed
12 tasks done

Extend the current DB layer with an SQLite interface #154

akegalj opened this issue Apr 4, 2019 · 3 comments
Assignees

Comments

@akegalj
Copy link
Contributor

akegalj commented Apr 4, 2019

Context

Our current implementation has db layer interface defined in https://github.com/input-output-hk/cardano-wallet/blob/master/src/Cardano/Wallet/DB.hs with MVar implementation of this interface defined in https://github.com/input-output-hk/cardano-wallet/blob/master/src/Cardano/Wallet/DB/MVar.hs . This MVar interface gives us ability to have in-memory db system. MVar was implemented because it was simplest thing to test the db layer design/interface. MVar db interface is also very useful because we can use it to run all db actions quick in memory which is useful for implementing tests.

To really have useful db system we have to implement db layer with one that will persist data to disk.

Decision

In a similar way we have added MVar implementation of db layer, add SQLite implementation of db layer. This implementation should use sqlite https://sqlite.org/index.html to persist data to disk. Research available haskell implementations of sqlite and decide which best suits our needs.

Acceptance Criteria

  1. DB layer must be implemented with sqlite interface
  2. SQLiteSpec module must contain tests for sqlite interface

Development Plan

  • Write SQL Schema using Persistent DSL
  • Implement DBLayer methods in using Persistent query functions
  • Add functions for creating a database connection, and running database migrations.
  • Add basic "sanity test" cases while implementing DBLayer methods, while DBLayer is not complete.
  • Make the MVarSpec tests run against the Sqlite DBLayer.
  • Fix test failures.
  • Generate more DBLayer test cases using quickcheck-state-machine.
  • Add performance testing to ensure the DBLayer still works with large numbers of wallets/addresses/transactions.
  • Use the Sqlite DBLayer by default when running cardano-wallet.
  • Command-line parameter to configure database filename.
  • (@KtorZ) enable SQLite in integration tests
  • (@KtorZ) Fix tx pending & UTxO selectors

PR

QA

  • Automated unit tests are in ./lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs and ./lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs.
  • Run the DBLayer benchmarks with stack bench cardano-wallet-core:db --ba "--output bench.html"
  • cardano-wallet now has a --database option. Without that option, it will use the SQLite in-memory database.
  • cardano-launcher now has a --state-dir option.
  • restoration benchmarks now also use the SQLite implementation instead of the MVar which allows for restoring in rather similar time but, with a much lower memory footprint (<350Mb for the 10% scheme on mainnet).
  • Integration tests also run using the SQLite engine.
@jonathanknowles
Copy link
Member

jonathanknowles commented May 14, 2019

@KtorZ @rvl @akegalj

Regarding concurrency:

  1. For the Sqlite implementation, are there any invariants or constraints that can only be maintained programmatically, in Haskell code, rather than by the underlying database implementation?
  2. For the functions provided by DBLayer (createWallet, removeWallet, listWallets, and so on) what level of concurrency do we need to provide to consumers of this API? Is the intention to allow certain operations to proceed in parallel? Or do we intend to place requests into some serialized order?
  3. Will we opt for a simple locking model (a big lock that must be taken out before performing an operation that could affect or inspect data affected by an invariant) or something more complex?

I imagine the answers to these questions might affect the design of our tests. For a simple locking model, it would mean that we only need to test linear sequences of arbitrary operations. However, if we need to allow greater levels of concurrency, then our testing would need to reflect this.

iohk-bors bot added a commit that referenced this issue May 20, 2019
277: Further enhancements to the Bech32 library. r=KtorZ a=jonathanknowles

## Issue Number

Issue #238 

## Overview

**This PR:**

1. Implements **error location detection** for the Bech32 encoding, based on the JavaScript [reference implementation](https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js).

2. Provides a set of **property tests** to verify that corrupted Bech32 strings are correctly **rejected** by the `decode` function. Corruption is simulated in various ways:
    * deleting characters
    * inserting characters
    * swapping characters
    * mutating characters

## Caveats

1. The error location detection is **best effort**:
    * In cases where it **is** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` error with a suitable list of locations.
    * In cases where it's **not** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` with the empty list.

2. Currently, there is no automated test to confirm that the `locateErrors` and `syndrome` functions behave identically to the equivalent functions in the reference JavaScript implementation. ⚠️  **It is possible that these implementations differ** (and this is something that we should pay attention to).

Nevertheless, in the test suite, we verify that **if** the position of an invalid character **is** detected, then it is **detected at the correct location**.

283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154. This PR branch is based on #282.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- I haven't made any decision about what to do with internal functions such as the `Wallet` constructor.
- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue May 20, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154. This PR branch is based on #282.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- I haven't made any decision about what to do with internal functions such as the `Wallet` constructor.
- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
rvl added a commit that referenced this issue May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154. This PR branch is based on #282.

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

- I haven't made any decision about what to do with internal functions such as the `Wallet` constructor.
- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)

Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
iohk-bors bot added a commit that referenced this issue May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
iohk-bors bot added a commit that referenced this issue May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


289: show feature availability in API specification r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

N/A

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have removed the"priority" from the specification and added a "status" indicating the availability of a particular feature in the API.

# Comments

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

As requested by Chris. Makes it easier for externals to know what's available or not. 

A preview:

![image](https://user-images.githubusercontent.com/5680256/58074137-bdc26f00-7ba4-11e9-95b5-33ff723e0f2a.png)

![image](https://user-images.githubusercontent.com/5680256/58074150-c6b34080-7ba4-11e9-83a5-943877e14fd7.png)

![image](https://user-images.githubusercontent.com/5680256/58074161-cfa41200-7ba4-11e9-8d83-0948b129e856.png)

![image](https://user-images.githubusercontent.com/5680256/58074177-dd599780-7ba4-11e9-867e-5047012b1f93.png)

![image](https://user-images.githubusercontent.com/5680256/58074188-e5193c00-7ba4-11e9-845c-cacfd34a9fc9.png)

<!-- 
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
 ✓ Acknowledge any changes required to the Wiki
-->


292: Fix Change Calculation (#286) r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#286 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended our test to capture the issue highlighted in the bug
- [x] I have looked at the test failing
- [x] I have fixed the calculation of the change which was... weirdly way off the specification..

# Comments

<!-- 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
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
iohk-bors bot added a commit that referenced this issue May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


289: show feature availability in API specification r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

N/A

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have removed the"priority" from the specification and added a "status" indicating the availability of a particular feature in the API.

# Comments

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

As requested by Chris. Makes it easier for externals to know what's available or not. 

A preview:

![image](https://user-images.githubusercontent.com/5680256/58074137-bdc26f00-7ba4-11e9-95b5-33ff723e0f2a.png)

![image](https://user-images.githubusercontent.com/5680256/58074150-c6b34080-7ba4-11e9-83a5-943877e14fd7.png)

![image](https://user-images.githubusercontent.com/5680256/58074161-cfa41200-7ba4-11e9-8d83-0948b129e856.png)

![image](https://user-images.githubusercontent.com/5680256/58074177-dd599780-7ba4-11e9-867e-5047012b1f93.png)

![image](https://user-images.githubusercontent.com/5680256/58074188-e5193c00-7ba4-11e9-845c-cacfd34a9fc9.png)

<!-- 
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
 ✓ Acknowledge any changes required to the Wiki
-->


292: Fix Change Calculation (#286) r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#286 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended our test to capture the issue highlighted in the bug
- [x] I have looked at the test failing
- [x] I have fixed the calculation of the change which was... weirdly way off the specification..

# Comments

<!-- 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
 ✓ Acknowledge any changes required to the Wiki
-->


296: Improve bech32 coveralls r=KtorZ a=piotr-iohk

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have replaced `isLeft` with `Left ...` here and there to improve coveralls score a bit


# Comments

<!-- 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
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>
Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>
@KtorZ
Copy link
Member

KtorZ commented Jun 12, 2019

I just run into some unexpected issues while running the integration tests scenarios using the SQLite engine. It seems that the way the checkpoint UTxO is retrieved is wrong and also include pending transaction. I am currently investigating / fixing this.

@piotr-iohk
Copy link
Contributor

This looks good 👍 .
I have added additional tests in #413 checking if --database and --state-dir produce files as expected.

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

6 participants