[AD-270] Check keyfile for unknown keys on startup #299

Haskell-mouse · 2018-09-04T14:17:11Z

YT issue: https://issues.serokell.io/issue/AD-270

Checklist:

Updated docs if necessary
Adressed HLint warnings and hints
Rebased branch on current master and squashed all "Address PR comment" commits
Tested my changes if they modify code

Description:

It's possible that key file has more secret keys than there are wallets in wallet DB. Btw, the opposite is impossible.

So - now unknown keys are deleted on startup.

gromakovsky

Looks good. Rejecting only because it conflicts with #288 and I'd like to merge that PR ASAP. Will approve after #288 is merged (hopefully tomorrow) and conflicts are resolved.

ariadne/cardano/src/Ariadne/Wallet/Backend.hs

artemohanjanyan

Otherwise, looks sensible

ariadne/cardano/src/Ariadne/MainTemplate.hs

ariadne/cardano/src/Ariadne/Wallet/Backend/KeyStorage.hs

gromakovsky · 2018-09-06T21:11:20Z

I suddenly realized that this solution is dangerous. If user somehow passes wrong path to wallet-db, all secret keys will be deleted and it may cause loss of funds. I see two options:

Use some heuristics to decide whether to delete secret keys (e. g. if set of keys is similar to set of wallets).
Insert into wallet DB first and delete from keyfile first. Then on startup we'll have to delete stuff from wallet DB, which is less dangerous.

Both options don't sound very good :/
I'll get back to it tomorrow.

Haskell-mouse · 2018-09-07T01:44:28Z

Maybe we shoud just check for empty wallet-db on startup and ask user about correctness of wallet-db path in case of empty wallet-db and non-empty keyfile ?
Another option is creating backup of keyfile in case of deletion of keys on startup.

gromakovsky · 2018-10-18T17:20:52Z

This is the thread where we discussed how to proceed with the issue: https://serokell.slack.com/archives/C9B1DAMDJ/p1536319212000100

gromakovsky

I haven't reviewed whole PR yet, but there already are some things which need to be changed. Will make another round of review later.
Next time you make a PR, please check that CI passes a bit later (when it finishes), currently there are some warnings and it fails.

stack-to-nix.yaml

ariadne/cardano/src/Ariadne/MainTemplate.hs

ariadne/cardano/src/Ariadne/Wallet/Backend/KeyStorage.hs

ariadne/cardano/src/Ariadne/MainTemplate.hs

ariadne/core/src/Ariadne/UIConfig.hs

Haskell-mouse · 2018-10-22T23:06:28Z

It is really strange (i am talking about CI), because i have "-Werror" option enabled in generated ".cabal" file an have no warnings. Will fix now.

gromakovsky · 2018-10-23T08:15:10Z

If you are using make dev or something similar, keep in mind that it passes -Wwarn to GHC.

Haskell-mouse · 2018-10-24T13:53:50Z

updated PR

ariadne/cardano/src/Ariadne/Wallet/Backend/KeyStorage.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/Wallets.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

ariadne/cardano/src/Ariadne/Wallet/Face.hs

ariadne/core/src/Ariadne/UIConfig.hs

ariadne/cardano/src/Ariadne/Wallet/Backend/KeyStorage.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/Wallets.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Kernel.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Types.hs

ariadne/core/src/Ariadne/UIConfig.hs

ui/vty-lib/src/Ariadne/UI/Vty/Widget/Dialog/ConfirmRemove.hs

ariadne/cardano/src/Ariadne/MainTemplate.hs

artemohanjanyan

I'll review the rest of the code a bit later. Couple of comments for now and this bug I noticed. Steps to reproduce:

Launch Ariadne.
new-wallet name: "A" no-confirm: true
Close Ariadne.
Open Ariadne.
Remove previously created wallet "A".
Close Ariadne.
Open Ariadne and behold some weird wallet and unknown keys dialog.

ariadne/cardano/src/Ariadne/Wallet/Backend.hs

ariadne/core/src/Ariadne/UIConfig.hs

Haskell-mouse · 2018-11-12T20:56:54Z

@artemohanjanyan
Can't reproduce the bug, tried it few times and have no that new weird wallet (

gromakovsky · 2018-11-13T14:04:03Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/Wallets.hs

@@ -192,10 +192,11 @@ deleteHdWallet pw rootId = do
            Keystore.delete (WalletIdHdSeq rootId) (pw ^. walletKeystore)
            return $ Right ()

-removeKeysFromKeyStore :: PassiveWallet
+removeKeysFromKeystore
+ :: PassiveWallet


Now indentation looks very weird.

Why was it marked as resolved? It's 1-space indentation here. It's not allowed.

It was marked as resolved because it is resolved. I made the same indentation as in other functions in the module. Here you see the old version before the last commit. Look

Hm, strange, I was pretty sure I was looking at the latest version last time I saw it. Ok then.

ariadne/cardano/src/Ariadne/MainTemplate.hs

gromakovsky · 2018-11-22T21:45:10Z

There's a new conflict, btw.

Haskell-mouse · 2018-11-23T09:35:41Z

Resolved

gromakovsky · 2018-11-23T12:29:40Z

ui/qt-lib/src/Ariadne/UI/Qt/Widgets/Dialogs/Delete.hs

+    QWidget.adjustSize delete
+  _ -> pass
+
+


Extra newline.

ui/vty-app/Glue.hs

ariadne/cardano/src/Ariadne/MainTemplate.hs

ariadne/cardano/src/Ariadne/Wallet/Backend/KeyStorage.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/Keystore.hs

ariadne/cardano/src/Ariadne/Wallet/Cardano/Kernel/Wallets.hs

gromakovsky · 2018-11-26T13:35:55Z

ariadne/cardano/src/Ariadne/Wallet/Cardano/WalletLayer/Kernel.hs

 import Ariadne.Wallet.Cardano.Kernel.DB.AcidState (DB, dbHdWallets, defDB)
 import Ariadne.Wallet.Cardano.Kernel.DB.Resolved (ResolvedBlock)
 import Ariadne.Wallet.Cardano.Kernel.Keystore (Keystore)
+
+import qualified Ariadne.Wallet.Cardano.Kernel.Keystore as Keystore (lookup)


Why was this import moved?

ui/qt-lib/src/Ariadne/UI/Qt/Widgets/Dialogs/Delete.hs

gromakovsky

Ok. @artemohanjanyan please take a look.

artemohanjanyan

I also suspect that some text is not entirely correct according to English, but I'm not sure.

artemohanjanyan · 2018-12-07T10:56:25Z

ariadne/core/src/Ariadne/UIConfig.hs

+
+rmBrokenWalletsIntroMkMessage :: Text
+rmBrokenWalletsIntroMkMessage = "There are wallets without corresponding secret key \
+    \in keyfile.\n Without secret keys you can do almost nothing with such wallets. \


I think that

Suggested change

\in keyfile.\n Without secret keys you can do almost nothing with such wallets. \

\in keyfile. Without secret keys you can do almost nothing with such wallets. \

is better, but 🤷‍♂️

Problem: Keyfile may contain unknown keys (from deleted wallets for example) Also sometimes we may have wallets without corresponding keys in keyfile (user can delete keys by hands). Solution: Now keyfile is checked for unknown keys on startup and user is asked if he wants to delete them. Also wallets are checked for existence of corresponding secret keys in keyfile. User will be asked if he wants to delete broken wallets (without secret key). Also added small chages to qt and vty UI - made text on deletion widget dependent on deleting item (and delete confirmation widget is used to ask user, with changed text on widget, and added new type of deleting item - unknownKeys and brokenWallets).

Haskell-mouse requested review from gromakovsky and artemohanjanyan September 4, 2018 14:17

Haskell-mouse requested a review from dniku as a code owner September 4, 2018 14:17

Haskell-mouse removed the request for review from dniku September 4, 2018 14:17

gromakovsky suggested changes Sep 4, 2018

View reviewed changes

ariadne/cardano/src/Ariadne/Wallet/Backend.hs Outdated Show resolved Hide resolved

artemohanjanyan reviewed Sep 5, 2018

View reviewed changes

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from 8c37986 to f8012ab Compare October 22, 2018 01:20

Haskell-mouse requested review from goodsoft and maksbotan as code owners October 22, 2018 01:21

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch 3 times, most recently from 8aa0ca1 to cb45051 Compare October 22, 2018 17:05

gromakovsky suggested changes Oct 22, 2018

View reviewed changes

gromakovsky reviewed Oct 22, 2018

View reviewed changes

ariadne/core/src/Ariadne/UIConfig.hs Outdated Show resolved Hide resolved

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch 2 times, most recently from ce005c6 to 1492c22 Compare October 24, 2018 02:09

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from 1492c22 to eb59171 Compare October 25, 2018 01:53

gromakovsky suggested changes Oct 25, 2018

View reviewed changes

gromakovsky reviewed Oct 25, 2018

View reviewed changes

ariadne/core/src/Ariadne/UIConfig.hs Outdated Show resolved Hide resolved

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch 3 times, most recently from dcdd264 to 702b754 Compare November 5, 2018 17:48

gromakovsky suggested changes Nov 6, 2018

View reviewed changes

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from 3eea9c6 to 0ffdd4b Compare November 8, 2018 12:58

gromakovsky reviewed Nov 11, 2018

View reviewed changes

ui/vty-lib/src/Ariadne/UI/Vty/Widget/Dialog/ConfirmRemove.hs Show resolved Hide resolved

gromakovsky reviewed Nov 12, 2018

View reviewed changes

ariadne/cardano/src/Ariadne/MainTemplate.hs Outdated Show resolved Hide resolved

artemohanjanyan self-requested a review November 12, 2018 12:37

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from f38ebb8 to 81b4579 Compare November 12, 2018 17:59

artemohanjanyan suggested changes Nov 12, 2018

View reviewed changes

ariadne/cardano/src/Ariadne/Wallet/Backend.hs Outdated Show resolved Hide resolved

ariadne/cardano/src/Ariadne/Wallet/Backend.hs Outdated Show resolved Hide resolved

ariadne/core/src/Ariadne/UIConfig.hs Outdated Show resolved Hide resolved

artemohanjanyan reviewed Nov 12, 2018

View reviewed changes

ariadne/core/src/Ariadne/UIConfig.hs Outdated Show resolved Hide resolved

gromakovsky reviewed Nov 13, 2018

View reviewed changes

ariadne/cardano/src/Ariadne/MainTemplate.hs Outdated Show resolved Hide resolved

dniku changed the title ~~[AD-270]Now keyfile is checked for unknown keys on startup~~ [AD-270] Now keyfile is checked for unknown keys on startup Nov 19, 2018

dniku changed the title ~~[AD-270] Now keyfile is checked for unknown keys on startup~~ [AD-270] Check keyfile for unknown keys on startup Nov 19, 2018

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch 4 times, most recently from 705f519 to dc53c2c Compare November 22, 2018 11:18

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from dc53c2c to cfdf857 Compare November 23, 2018 08:30

gromakovsky reviewed Nov 23, 2018

View reviewed changes

ui/qt-lib/src/Ariadne/UI/Qt/Widgets/Dialogs/Delete.hs Outdated

QWidget.adjustSize delete

_ -> pass

Copy link

Member

gromakovsky Nov 23, 2018

Choose a reason for hiding this comment

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

Extra newline.

gromakovsky reviewed Nov 23, 2018

View reviewed changes

ui/vty-app/Glue.hs Show resolved Hide resolved

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch 2 times, most recently from ee01271 to 937633a Compare November 25, 2018 05:25

gromakovsky reviewed Nov 26, 2018

View reviewed changes

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from bd5e4c8 to 9f7d683 Compare November 30, 2018 09:29

gromakovsky approved these changes Nov 30, 2018

View reviewed changes

artemohanjanyan approved these changes Dec 7, 2018

View reviewed changes

Haskell-mouse force-pushed the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch from 9f7d683 to 6caec98 Compare December 13, 2018 18:31

Haskell-mouse merged commit 6caec98 into master Dec 13, 2018

serokell-bot deleted the Haskell-mouse/ad270-Delete-useless-secret-keys-on-start branch December 13, 2018 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AD-270] Check keyfile for unknown keys on startup #299

[AD-270] Check keyfile for unknown keys on startup #299

Haskell-mouse commented Sep 4, 2018 •

edited

Loading

gromakovsky left a comment

artemohanjanyan left a comment

gromakovsky commented Sep 6, 2018

Haskell-mouse commented Sep 7, 2018

gromakovsky commented Oct 18, 2018

gromakovsky left a comment

Haskell-mouse commented Oct 22, 2018

gromakovsky commented Oct 23, 2018

Haskell-mouse commented Oct 24, 2018

artemohanjanyan left a comment •

edited

Loading

Haskell-mouse commented Nov 12, 2018

gromakovsky Nov 13, 2018

gromakovsky Nov 23, 2018

Haskell-mouse Nov 25, 2018 •

edited

Loading

gromakovsky Nov 26, 2018

gromakovsky commented Nov 22, 2018

Haskell-mouse commented Nov 23, 2018

gromakovsky Nov 23, 2018

gromakovsky Nov 26, 2018

gromakovsky left a comment

artemohanjanyan left a comment

artemohanjanyan Dec 7, 2018

	\in keyfile.\n Without secret keys you can do almost nothing with such wallets. \
	\in keyfile. Without secret keys you can do almost nothing with such wallets. \

[AD-270] Check keyfile for unknown keys on startup #299

[AD-270] Check keyfile for unknown keys on startup #299

Conversation

Haskell-mouse commented Sep 4, 2018 • edited Loading

gromakovsky left a comment

Choose a reason for hiding this comment

artemohanjanyan left a comment

Choose a reason for hiding this comment

gromakovsky commented Sep 6, 2018

Haskell-mouse commented Sep 7, 2018

gromakovsky commented Oct 18, 2018

gromakovsky left a comment

Choose a reason for hiding this comment

Haskell-mouse commented Oct 22, 2018

gromakovsky commented Oct 23, 2018

Haskell-mouse commented Oct 24, 2018

artemohanjanyan left a comment • edited Loading

Choose a reason for hiding this comment

Haskell-mouse commented Nov 12, 2018

gromakovsky Nov 13, 2018

Choose a reason for hiding this comment

gromakovsky Nov 23, 2018

Choose a reason for hiding this comment

Haskell-mouse Nov 25, 2018 • edited Loading

Choose a reason for hiding this comment

gromakovsky Nov 26, 2018

Choose a reason for hiding this comment

gromakovsky commented Nov 22, 2018

Haskell-mouse commented Nov 23, 2018

gromakovsky Nov 23, 2018

Choose a reason for hiding this comment

gromakovsky Nov 26, 2018

Choose a reason for hiding this comment

gromakovsky left a comment

Choose a reason for hiding this comment

artemohanjanyan left a comment

Choose a reason for hiding this comment

artemohanjanyan Dec 7, 2018

Choose a reason for hiding this comment

Haskell-mouse commented Sep 4, 2018 •

edited

Loading

artemohanjanyan left a comment •

edited

Loading

Haskell-mouse Nov 25, 2018 •

edited

Loading