qt: remove unused 'Decrypt' wallet

sehyunc commented at 11:03 pm on July 31, 2020: contributor

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

remove unused decrypt case b3e566a6f4

change decrypt case in walletview dialog f9d41dacff

remove decrypt case from switch statements fb5b460e78

hebasto commented at 11:11 am on August 1, 2020: member

Concept ACK.

Saibato commented at 11:16 am on August 1, 2020: contributor

Concept tACK https://github.com/bitcoin-core/gui/pull/42/commits/f9d41dacff0d9d3bd2985966a19ee8d6a99c0b28

minor nit how about this?

 0
 1index 26aa38320..2ba74082f 100644
 2--- a/src/qt/walletview.cpp
 3+++ b/src/qt/walletview.cpp
 4@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
 5 {
 6     if(!walletModel)
 7         return;
 8-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
 9-    dlg.setModel(walletModel);
10-    dlg.exec();
11+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
12+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
13+        dlg.setModel(walletModel);
14+        dlg.exec();
15+    }
16 
17     updateEncryptionStatus();
18 }

hebasto commented at 11:58 am on August 1, 2020: member

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 commented at 12:02 pm on August 1, 2020: member

@Saibato

Concept tACK f9d41da

minor nit how about this?

 0index 26aa38320..2ba74082f 100644
 1--- a/src/qt/walletview.cpp
 2+++ b/src/qt/walletview.cpp
 3@@ -262,9 +262,11 @@ void WalletView::encryptWallet(bool status)
 4 {
 5     if(!walletModel)
 6         return;
 7-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Encrypt, this);
 8-    dlg.setModel(walletModel);
 9-    dlg.exec();
10+    if (walletModel->getEncryptionStatus() == WalletModel::Unencrypted) {
11+        AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
12+        dlg.setModel(walletModel);
13+        dlg.exec();
14+    }
15 
16     updateEncryptionStatus();
17 }

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

Saibato commented at 5:54 pm on August 1, 2020: contributor

@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 commented at 11:55 am on August 2, 2020: member

@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:

Saibato commented at 4:47 pm on August 2, 2020: contributor

Mind providing steps to reproduce it? @hebasto sure if iirc, compile i.e. with the PR or just

0  `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 commented at 11:17 pm on August 2, 2020: member

@Saibato

Testing fb5b460e7882eec85373bca0f41d66170392fe1c 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 :)

Screenshot from 2020-08-03 02-16-01

Saibato commented at 6:20 am on August 3, 2020: contributor

@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?

checkwallet

hebasto commented at 9:47 am on August 3, 2020: member

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

achow101 commented at 4:48 pm on August 3, 2020: member

Concept ACK

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

MarcoFalke commented at 5:43 pm on August 3, 2020: contributor

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

hebasto commented at 7:37 am on September 9, 2020: member

@sehyunc Are you going to address the latest comment?

MarcoFalke added the label up for grabs on Sep 9, 2020

MarcoFalke added the label Waiting for author on Sep 9, 2020

sehyunc commented at 5:15 pm on September 11, 2020: contributor

@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 commented at 5:13 pm on October 23, 2020: member

Fellow reviewers! Please review #109.

MarcoFalke referenced this in commit 54532f46c4 on Nov 18, 2020

MarcoFalke closed this on Nov 18, 2020

laanwj referenced this in commit 90c0f267bd on Dec 9, 2020

jarolrod commented at 7:27 pm on March 4, 2021: member

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

hebasto removed the label Waiting for author on Mar 4, 2021

hebasto removed the label up for grabs on Mar 4, 2021

bitcoin-core locked this on Aug 16, 2022

qt: remove unused ‘Decrypt’ wallet #42