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

[DEVOPS-1250] backport RCD-47 fix about 'hasSpendingPassword' metadata with extra sanity check #4112

Merged

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Mar 21, 2019

Description

Since a few users have already ran the migration, they might have migrated wallet with incorrect metadata, wrongly stating that
their wallet has no spending password, and as a consequence, preventing them from doing anything with their wallet. So, we do now
run an extra sanity check upon every start to make sure that the wallet acid state metadata matches what we find in the keystore.

Linked issue

[DEVOPS-150]

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

Creating Wallets on 1.3.x

$ curl -vkX POST "http://localhost:8090/api/v1/wallets" \
  -H "Content-Type: application/json;charset=utf-8" \
  -d '{
  "operation": "create",
  "spendingPassword": "",
  "backupPhrase": [...],
  "assuranceLevel": "normal",
  "name": "Without Password"
}' | jq .data

{
  "id": "Ae2tdPwUPEZ1UMga1bnFFD7FoFN9ZFoPLCWLaGcqNnfovcrcT3oMSuAz8DD",
  "name": "Without Password",
  "hasSpendingPassword": false,
  [..]
}
$ curl -vkX POST "http://localhost:8090/api/v1/wallets" \
  -H "Content-Type: application/json;charset=utf-8" \
  -d '{
  "operation": "create",
  "backupPhrase": [...],
  "spendingPassword": "5416b2988745725998907addf4613c9b0764f04959030e1b81c603b920a115d0",
  "assuranceLevel": "normal",
  "name": "With Password"
}' | jq .data

{
    "id": "Ae2tdPwUPEYvuFeJoBAiYtbFWaZwzya1om6Mqvbz9aL85TncK47bRhL4oZv",
    "name": "With Password",
    "hasSpendingPassword": true,
    [..]
}

Migrating wallet (without patch) from 1.3.x --> 3.0.x

Logs

[2019-03-21 11:32:50.15 UTC] Migrating HdRoot { id: HdRootId Ae2tdPw...SuAz8DD, name: <Migrated Wallet>, hasPassword: no ... }
[2019-03-21 11:32:51.27 UTC] Migrating HdRoot { id: HdRootId Ae2tdPw...RhL4oZv, name: <Migrated Wallet>, hasPassword: no ... }
[2019-03-21 11:32:52.14 UTC] Migration succeeded, restoration of migrated wallets in progress...

Wallet State

$ curl -kX GET http://localhost:8090/api/v1/wallets | jq .data
[
  {
    "id": "Ae2tdPwUPEZ1UMga1bnFFD7FoFN9ZFoPLCWLaGcqNnfovcrcT3oMSuAz8DD",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": false,
    [...]
  },
  {
    "id": "Ae2tdPwUPEYvuFeJoBAiYtbFWaZwzya1om6Mqvbz9aL85TncK47bRhL4oZv",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": false,
    [...]
  }
]

Restarting (with patch) an already migrated wallet (without patch) from 1.3.x --> 3.0.x

Logs

[cardano-sl.node:Info:ThreadId 1727] [2019-03-21 11:42:23.90 UTC] Starting wallet(s) migration
[cardano-sl.node:Info:ThreadId 1727] [2019-03-21 11:42:23.90 UTC] No legacy DB at state-devops1250/wallet-db, migration is not needed.

Wallet State

$ curl -kX GET http://localhost:8090/api/v1/wallets | jq .data
[
  {
    "id": "Ae2tdPwUPEZ1UMga1bnFFD7FoFN9ZFoPLCWLaGcqNnfovcrcT3oMSuAz8DD",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": false,
    [...]
  },
  {
    "id": "Ae2tdPwUPEYvuFeJoBAiYtbFWaZwzya1om6Mqvbz9aL85TncK47bRhL4oZv",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": true,
    [...]
  }
]

Migrating wallet (with patch) from 1.3.x --> 3.0.x

Logs

[2019-03-21 11:49:03.24 UTC] Migrating HdRoot { id: HdRootId Ae2tdPw...SuAz8DD, name: <Migrated Wallet>, hasPassword: no ... }
[2019-03-21 11:49:04.50 UTC] Migrating HdRoot { id: HdRootId Ae2tdPw...RhL4oZv, name: <Migrated Wallet>, hasPassword: updated 1553168943743216 ... }
[2019-03-21 11:49:04.50 UTC] Migration succeeded, restoration of migrated wallets in progress...

Wallet State

$ curl -kX GET http://localhost:8090/api/v1/wallets | jq .data
[
  {
    "id": "Ae2tdPwUPEZ1UMga1bnFFD7FoFN9ZFoPLCWLaGcqNnfovcrcT3oMSuAz8DD",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": false,
    [..]
  },
  {
    "id": "Ae2tdPwUPEYvuFeJoBAiYtbFWaZwzya1om6Mqvbz9aL85TncK47bRhL4oZv",
    "name": "<Migrated Wallet>",
    "hasSpendingPassword": true,
    [...]
  }
]

Screenshots (if available)

How to merge

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

@KtorZ KtorZ self-assigned this Mar 21, 2019
@KtorZ KtorZ requested a review from cleverca22 March 21, 2019 10:44
@KtorZ KtorZ force-pushed the KtorZ/DEVOPS-1250/has-spending-password-metadata-consistency branch from 784b416 to 434eae1 Compare March 21, 2019 10:46
KtorZ added 2 commits March 21, 2019 12:31
…a with extra sanity check

Since a few users have already ran the migration, they might have migrated wallet with incorrect metadata, wrongly stating that
their wallet has no spending password, and as a consequence, preventing them from doing anything with their wallet. So, we do now
run an extra sanity check upon every start to make sure that the wallet acid state metadata matches what we find in the keystore.
@KtorZ KtorZ force-pushed the KtorZ/DEVOPS-1250/has-spending-password-metadata-consistency branch from 434eae1 to af431e8 Compare March 21, 2019 11:31
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM, pending QA.

forM_ wKeys $ \esk -> do
let hasSpendingPassword = case checkPassMatches mempty esk of
Nothing -> HD.HasSpendingPassword lastUpdateNow
Just _ -> HD.NoSpendingPassword
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, ok. So the Just case is when the password does match the empty password, hence NoSpendingPassword.

(_, Left e) ->
return $ Left e
(HD.HasSpendingPassword _, Right root) | root ^. HD.hdRootHasPassword == HD.NoSpendingPassword ->
action rootId hasSpendingPassword
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, logic seems right here too.

Copy link
Contributor Author

@KtorZ KtorZ Mar 21, 2019

Choose a reason for hiding this comment

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

It's not entirely necessary but I thought it would be a good idea to only make updates to the DB when necessary.

@disassembler
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Mar 21, 2019
4112: [DEVOPS-1250] backport RCD-47 fix about 'hasSpendingPassword' metadata with extra sanity check r=disassembler a=KtorZ

## Description

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

Since a few users have already ran the migration, they might have migrated wallet with incorrect metadata, wrongly stating that
their wallet has no spending password, and as a consequence, preventing them from doing anything with their wallet. So, we do now
run an extra sanity check upon every start to make sure that the wallet acid state metadata matches what we find in the keystore.


## Linked issue

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

[[DEVOPS-150]](https://iohk.myjetbrains.com/youtrack/issue/DEVOPS-1250)



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

iohk-bors bot commented Mar 21, 2019

@iohk-bors iohk-bors bot merged commit 7877219 into develop Mar 21, 2019
@iohk-bors iohk-bors bot deleted the KtorZ/DEVOPS-1250/has-spending-password-metadata-consistency branch March 21, 2019 17:02
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.

3 participants