Skip to content
This repository was archived by the owner on Mar 1, 2019. It is now read-only.

Fix Metadata Inconsistency After Wallet or Account Deletion #146

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Dec 10, 2018

Linked Issue

#141

Overview

  • 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 concurrent 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 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.

  • It now ignores insertion violations thay may occur if a user restore a deleted wallet or account. Cleaning up the metadata table itself isn't sufficient; we also need (in theory) to cleanup the corresponding inputs and outputs though in practice, this isn't that simple. Inputs may be used by different tx. Ideally, we would rely on db foreign keys for that but beam (the library we use to talk to SQLite) doesn't support them... Therefore, dust remains in the DB after deletion which is acceptable for now.

  • It adds a few regression integration scenarios to illustrate the various fixes.

Comments

This is actually a port of cardano-sl#3943 already reviewed and discussed with Kostas.

Checklist

  • I've kept this PR simple, on-the-spot, free of cosmetic changes.
  • I have reviewed my commit messages and history.
  • I've assigned a reviewer.
  • I acknowledge my changes may require an update in the wiki.
  • I have added a reference to this PR to the corresponding issue.

  • I've checked the checklist

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.
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.

The PR introduces precise clearing of meta that is needed in a number of cases, for instance, when the wallet is deleted and later comes back or when the account of the wallet is deleted. Without this the respective metadata was inconsistent and results in misleading response and behavior. The exact scenarios targeted are "spelled and written down" in the integration test.

I am very fond of this way of addressing errors in the integration tests. It boosts confidence that the problem was treated properly, makes easier to review the PR and make sure the same bug is not going to elude us later and resurface. Probably it should be a requirement to add the corresponding integration tests for each error/bug not covered.

@KtorZ KtorZ merged commit 51dad71 into develop Dec 11, 2018
@KtorZ KtorZ deleted the KtorZ/141/fix-metadata-inconsistency-after-deletion branch December 11, 2018 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants