Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-495] Fix inconsistent metadata store after deletion #3943

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Dec 8, 2018

Description

We store some information related to transaction in the metadata store (sqlite). However, when looking up transaction with metadata referring to wallet we don't know about, we fail with a not so friendly error "WalletNotFound" despite no wallet being given as part of the query.

This fix is actually two folds:

  • It discards incoherent transactions fetched from the DB, if any,
    and shout a warning in the log. This is in order to make the system
    more resilient to conconcurrent calls while a wallet or account is
    being deleted (since metadata and accounts / wallets are stored in
    separated databases, we can't easily run both delete in a single
    transaction).

  • It also deletes corresponding metadata when an account or a wallet
    is removed. This may cause extra damage? What if there are pending
    transactions when we delete the account or wallet.

Linked issue

CBR-495

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

On input-output-hk/cardano-sl I've added the following integration tests:

    scenario "Reg#141: metadata become inconsistent after a wallet is deleted" $ do
        fixture <- setup $ defaultSetup
            & initialCoins .~ [14]

        response <- request $ Client.getTransactionIndexFilterSorts
            $- Nothing
            $- Nothing
            $- Nothing
            $- defaultPage
            $- defaultPerPage
            $- NoFilters
            $- NoSorts

        verify response
            [ expectSuccess
            ]

    scenario "Reg#141: metadata become inconsistent after an account is deleted" $ do
        fixture <- setup $ defaultSetup
            & initialCoins .~ [500000]

        request_ $ Client.postAccount
            $- fixture ^. wallet . walletId
            $- NewAccount
                noSpendingPassword
                "My Second Account"

        request_ $ Client.deleteAccount
            $- fixture ^. wallet . walletId
            $- defaultAccountId

        response <- request $ Client.getTransactionIndexFilterSorts
            $- Just (fixture ^. wallet . walletId)
            $- Nothing
            $- Nothing
            $- defaultPage
            $- defaultPerPage
            $- NoFilters
            $- NoSorts

        verify response
            [ expectSuccess
            ]

Both now passes. Beside, in case of future inconsistency like this in the DB, the whole endpoint won't fail but instead, inconsistent transactions will be discarded and a warning printed in the logs:

[cardano-sl.walletapi:Info:ThreadId 1201] [2018-12-08 15:05:49.40 UTC] 
GET Request #28
    :> api
    :> v1
    :> transactions
    :> 'wallet_id' field: -
    :> 'account_index' field: -
    :> 'address' field: -
    :> pagination: page=1, per_page=10
    :> filter_by: -
    :> sort_by: -
[cardano-sl.edge:Warning:ThreadId 1201] [2018-12-08 15:05:49.40 UTC] Inconsistent entry in the metadata store: UnknownHdAccountRoot HdRootId Ae2tdPwUPEZKUpTf4jDurpvX7fdUrmZvFPM56TAwihnBbAuyvMZ47NeHHdY
[cardano-sl.walletapi:Info:ThreadId 1201] [2018-12-08 15:05:49.40 UTC] 
    Response #28 OK 0.002410603s

Screenshots (if available)

How to merge

Send the message bors r+ to merge this PR. For more information, see
docs/how-to/bors.md.

This fix is actually two folds:

- It discards incoherent transactions fetched from the DB, if any,
  and shout a warning in the log. This is in order to make the system
  more resilient to conconcurrent calls while a wallet or account is
  being deleted (since metadata and accounts / wallets are stored in
  separated databases, we can't easily run both delete in a single
  transaction).

- It also deletes corresponding metadata when an account or a wallet
  is removed. This may cause extra damage? What if there are pending
  transactions when we delete the account or wallet.
@KtorZ KtorZ requested a review from kderme December 8, 2018 16:46
@KtorZ KtorZ changed the title Ktor z/cbr 495/fix inconsistent metadata store after deletion [CBR-495] Fix inconsistent metadata store after deletion Dec 8, 2018
@KtorZ
Copy link
Contributor Author

KtorZ commented Dec 10, 2018

Added an extra integration test to illustrate last comment and @kderme addition.

    scenario "Reg#141: metadata becomes inconsistent after a wallet is deleted and restored" $ do
        fixture <- setup $ defaultSetup
            & initialCoins .~ [500000]

        [verifyTxInHistory, verifyWalletIsRestored] <- sequence
            [ do -- // 1 Make sure there's a transaction in the history
                response <- request $ Client.postTransaction $- Payment
                    (defaultSource fixture)
                    (defaultDistribution 42 fixture)
                    defaultGroupingPolicy
                    noSpendingPassword
                verify response
                    [ expectTxStatusEventually (fixture ^. wallet) [InNewestBlocks, Persisted]
                    ]
                -- // 3 Later, check that we can recover that transaction
                return $ verify response
                    [ expectTxInHistoryOf  (fixture ^. wallet)
                    ]
            , do -- // 2 Remove and then, restore the wallet
                request_ $ Client.deleteWallet $- fixture ^. wallet . walletId
                response <- request $ Client.postWallet $- NewWallet
                    (fixture ^. backupPhrase)
                    noSpendingPassword
                    defaultAssuranceLevel
                    defaultWalletName
                    RestoreWallet
                return $ verify response
                    [ expectWalletEventuallyRestored
                    ]
            ]
        verifyWalletIsRestored >> verifyTxInHistory

Succeeds with the last addition, but otherwise fails with:

  test/integration/Test/Integration/Framework/DSL.hs:131:23: 
  1) Transactions Reg#141: metadata becomes inconsistent after a wallet is deleted and restored
       uncaught exception: IOException of type UserError
       user error (expectWalletEventuallyRestored: waited too long for restoration.)

-- We handle Nothing and (Just True) the same here, since
-- it's possible that there is no Meta with this Inputs/Outputs.
-- In the future we may consider doing a better cleanup to avoid
-- such cases.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another integration test added to cover these bits 👍 , cf comments.

@KtorZ
Copy link
Contributor Author

KtorZ commented Dec 10, 2018

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 10, 2018
3943: [CBR-495] Fix inconsistent metadata store after deletion r=KtorZ a=KtorZ

## Description

We store some information related to transaction in the metadata store (sqlite). However, when looking up transaction with metadata referring to wallet we don't know about, we fail with a not so friendly error "WalletNotFound" despite no wallet being given as part of the query.

This fix is actually two folds:

- It discards incoherent transactions fetched from the DB, if any,
  and shout a warning in the log. This is in order to make the system
  more resilient to conconcurrent calls while a wallet or account is
  being deleted (since metadata and accounts / wallets are stored in
  separated databases, we can't easily run both delete in a single
  transaction).

- It also deletes corresponding metadata when an account or a wallet
  is removed. This may cause extra damage? What if there are pending
  transactions when we delete the account or wallet.

## Linked issue

[CBR-495](https://iohk.myjetbrains.com/youtrack/issue/CBR-495)



3947: [CBR-496] Fix --rebuild-db not rebuilding sqlite r=jmitchell a=KtorZ

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

Turns out we aren't actually removing the sqlite database when passing --rebuild-db becaue the SQLite db is actually stored in a file and not a directory. 

## Linked issue

<!--- Put here the relevant issue from YouTrack -->

[[CBR-496]](https://iohk.myjetbrains.com/youtrack/issue/CO-445)



Co-authored-by: KtorZ <[email protected]>
Co-authored-by: kderme <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 10, 2018

Timed out (retrying...)

iohk-bors bot added a commit that referenced this pull request Dec 10, 2018
3943: [CBR-495] Fix inconsistent metadata store after deletion r=KtorZ a=KtorZ

## Description

We store some information related to transaction in the metadata store (sqlite). However, when looking up transaction with metadata referring to wallet we don't know about, we fail with a not so friendly error "WalletNotFound" despite no wallet being given as part of the query.

This fix is actually two folds:

- It discards incoherent transactions fetched from the DB, if any,
  and shout a warning in the log. This is in order to make the system
  more resilient to conconcurrent calls while a wallet or account is
  being deleted (since metadata and accounts / wallets are stored in
  separated databases, we can't easily run both delete in a single
  transaction).

- It also deletes corresponding metadata when an account or a wallet
  is removed. This may cause extra damage? What if there are pending
  transactions when we delete the account or wallet.

## Linked issue

[CBR-495](https://iohk.myjetbrains.com/youtrack/issue/CBR-495)



Co-authored-by: KtorZ <[email protected]>
Co-authored-by: kderme <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 10, 2018

Timed out

@jmitchell
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 10, 2018
3943: [CBR-495] Fix inconsistent metadata store after deletion r=jmitchell a=KtorZ

## Description

We store some information related to transaction in the metadata store (sqlite). However, when looking up transaction with metadata referring to wallet we don't know about, we fail with a not so friendly error "WalletNotFound" despite no wallet being given as part of the query.

This fix is actually two folds:

- It discards incoherent transactions fetched from the DB, if any,
  and shout a warning in the log. This is in order to make the system
  more resilient to conconcurrent calls while a wallet or account is
  being deleted (since metadata and accounts / wallets are stored in
  separated databases, we can't easily run both delete in a single
  transaction).

- It also deletes corresponding metadata when an account or a wallet
  is removed. This may cause extra damage? What if there are pending
  transactions when we delete the account or wallet.

## Linked issue

[CBR-495](https://iohk.myjetbrains.com/youtrack/issue/CBR-495)



3947: [CBR-496] Fix --rebuild-db not rebuilding sqlite r=jmitchell a=KtorZ

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

Turns out we aren't actually removing the sqlite database when passing --rebuild-db becaue the SQLite db is actually stored in a file and not a directory. 

## Linked issue

<!--- Put here the relevant issue from YouTrack -->

[[CBR-496]](https://iohk.myjetbrains.com/youtrack/issue/CO-445)



Co-authored-by: KtorZ <[email protected]>
Co-authored-by: kderme <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 11, 2018

@iohk-bors iohk-bors bot merged commit a74f4a0 into release/2.0.0 Dec 11, 2018
@iohk-bors iohk-bors bot deleted the KtorZ/CBR-495/fix-inconsistent-metadata-store-after-deletion branch December 11, 2018 00:22
@KtorZ KtorZ mentioned this pull request Jan 4, 2019
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants