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 SQLite in integration tests scenarios (instead of MVar) #406

Merged
merged 8 commits into from
Jun 13, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jun 12, 2019

Issue Number

#154

Overview

  • I have enabled SQLite for the integration tests instead of the MVar impl.
  • I have fixed a few issues discovered by running the integration scenarios:
    • We used to store the totalUTxO when storing the checkpoint, which is a modified version of the actual checkpoint UTxO. Fixed by providing a new accessor to get the "raw" UTxO of a checkpoint.
    • There was a PendingTx table storing pending transactions identified by the wallet id and a slot. That is redundant with the TxMeta already stored, plus, it was not maintained properly and wouldn't yield the correct pending txs when restoring a checkpoint.
    • We need to also use repsert inps and outs when inserting pending txs of a checkpoint because inputs or outputs may have been inserted by some previous transactions!
    • Correctly account for pending transaction in the state-machine mock model.
    • Discard pending transaction in the isolation property (there's no isolation here when inserting a pending tx, we expect the checkpoint to have changed to account for it).

Comments

It'd be nice to find some sort of invariant to enforce regarding pending txs and the corresponding checkpoint. We don't expect putTxHistory to be called without a putCheckpoint following either before or after. One way to deal with this could be:

  • Enforce that putTxHistory is never called with pending txs
  • Store TxMeta in the checkpoint, such that putCheckpoint has enough information to fully store a pending tx.

edit: Addressed points above in a last commit.

⚠️ As always, reviewing commit by commit will make it easier! ⚠️

@KtorZ KtorZ self-assigned this Jun 12, 2019
@KtorZ KtorZ requested a review from paweljakubas June 12, 2019 15:54
@KtorZ KtorZ force-pushed the KtorZ/sqlite-in-integration-scenarios branch from 14fcfeb to 9ae58b8 Compare June 12, 2019 16:05
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.

Cool, removing the pending table simplifies things.

lib/core/src/Cardano/Wallet/DB/Sqlite.hs Outdated Show resolved Hide resolved
@KtorZ
Copy link
Member Author

KtorZ commented Jun 13, 2019

Seems like the integration tests are hanging forever in case of failure, like something preventing the thread from dying. I guess that because the SQLite connection is never closed. I'll have a look.

KtorZ added 6 commits June 13, 2019 09:34
…rmed UTxO from a checkpoint

The SQLite implementation was storing 'totalUTxO' which is completely invalid since this:
    - does not contain the pending UTxO
    - does contain the change UTxO

So, after storing a checkpoint and recovering it from the DB, we'll end up with the same UTxO as-if all pending
txs were accepted and not pending anymore.
…-machine when reading checkpoint

Pending transactions can actually be inserted independently of the
wallet checkpoints. So, if we insert a checkpoint after we inserted
some pending txs, reading that checkpoint again should yield those
pending txs.
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM module the hanging tests upon fail

@KtorZ KtorZ force-pushed the KtorZ/sqlite-in-integration-scenarios branch from 378313f to f79f3a3 Compare June 13, 2019 08:32
@KtorZ KtorZ force-pushed the KtorZ/sqlite-in-integration-scenarios branch from f79f3a3 to 39bcc46 Compare June 13, 2019 08:57
@KtorZ KtorZ merged commit ba5dba6 into master Jun 13, 2019
@KtorZ KtorZ deleted the KtorZ/sqlite-in-integration-scenarios branch June 13, 2019 09:21
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

Successfully merging this pull request may close these issues.

4 participants