qt: remove unused 'Decrypt' wallet

[sehyunc]

Fix #1
remove 'Decrypt' case from enum in AskPassPhraseDialog
remove 'Decrypt' case from switch statements
show only Encrypt option in dialog
would appreciate feedback, specifically on src/qt/walletview.cpp:265

[hebasto]

Concept ACK.

[Saibato]

Concept tACK f9d41da

minor nit how about this?

index 26aa38320..2ba74082f 100644
--- a/src/qt/walletview.cpp
+++ b/src/qt/walletview.cpp
@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
 {
     if(!walletModel)
         return;
-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
-    dlg.setModel(walletModel);
-    dlg.exec();
+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
+        dlg.setModel(walletModel);
+        dlg.exec();
+    }
 
     updateEncryptionStatus();
 }

[hebasto]

As a general rule each commit must compile and pass tests. Therefore, please squash all commits into a single one.

Mind refactoring enum Mode into enum class Mode? (this suggestion could be leaved for an followup though)

Since the encryptWalletAction is enabled only after encryptWalletAction->setChecked(false) (see: BitcoinGUI::setEncryptionStatus()), may I suggest to explicitly ignore the bool parameter (or, better, assert(!status)) in WalletFrame::encryptWallet(), and make WalletView::encryptWallet() parameterless?

[hebasto]

@Saibato

Concept tACK f9d41da

minor nit how about this?

index 26aa38320..2ba74082f 100644
--- a/src/qt/walletview.cpp
+++ b/src/qt/walletview.cpp
@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
 {
     if(!walletModel)
         return;
-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
-    dlg.setModel(walletModel);
-    dlg.exec();
+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
+        dlg.setModel(walletModel);
+        dlg.exec();
+    }
 
     updateEncryptionStatus();
 }

Condition walletModel->getEncryptionStatus() == WalletModel::Unencrypted is always true here, right? Why do we need to add it?

[Saibato]

@hebasto tyi , not what I observed, when testing, maybe a minor bug?

always true here, right? Why do we need to add it?

The flag is an int and give us the info we need here.

The bool does sometimes not correctly reflect all three states.
And then you could encrypt and encrypted again, what makes not that much sense.
Could be a minor bug that sets the status wrong before call?

Maybe @sehyunc like to find out as an exercise?

Did not dig deeper. tyi

[hebasto]

@Saibato

@hebasto tyi , not what I observed, when testing, maybe a minor bug?

Mind providing steps to reproduce it?

The bool does sometimes not correctly reflect all three states.

The bool is just a value of encryptWalletAction->checked(), see:

• https://doc.qt.io/qt-5/qaction.html#checked-prop
• https://doc.qt.io/qt-5/qaction.html#triggered

[Saibato]

Mind providing steps to reproduce it?

@hebasto sure if iirc, compile i.e. with the PR or just

  `AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this) `

then create multiple wallets load all encrypt all, close all and then load them randomly and then try click encrypt. You probably will find by that even the next bug not mentioned yet, hoops you asked, I like that ping pong push things forward.
And as they say, its a marathon not a sprint,
One runs the other backup and then baton or name/avatar change before the bugs hit in.

[hebasto]

@Saibato

Testing fb5b460 on Linux Mint 20 (Qt 5.12.8)...

then create multiple wallets load all encrypt all

... done...

close all

... done...

and then load them randomly

... done...

and then try click encrypt.

I cannot click "Encrypt Wallet" menu item because it is disabled. As expected :)

[Saibato]

@hebasto
And i could, so could it be since that now has happened more than often that your configuration and testing setup, could not confirm things,

So you as somewhat main gui dev should maybe switch to common used platforms ubuntu/lubuntu/debian/mac with there default qt dev libs or could be we have here a hardware dependent racing condition?

Or maybe I have the most exotic combination and the AI that simulates me is not efficient or fast enough to guess how it must simulate the desktop my virtual eye's will see?

[hebasto]

@Saibato
The bug you described fixed in #43. Mind testing it?

[achow101]

Concept ACK

As noted by others, the commit order should be reversed or all the commits squashed so that each commit individually compiles.

[maflcko]

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[hebasto]

@sehyunc Are you going to address the latest comment?

[sehyunc]

@sehyunc Are you going to address the latest comment?

Yes, apologies, have been caught up in school work. Will try my best to push a new commit.

[hebasto]

Fellow reviewers! Please review #109.

[jarolrod]

@MarcoFalke @hebasto can the waiting for author and up for grab labels be removed from here as to not confuse anyone who may start on this by accident.