Respect dialog modality and fix a regression in wallet unlock

hebasto commented at 5:04 pm on December 17, 2021: member

As pointed in bitcoin/bitcoin#23790 a regression in wallet unlock was introduced in bitcoin-core/gui#336 when a synchronous AskPassphraseDialog has been replaced with an asynchronous one.

This PR reverts a call back to a synchronous mode.

To make synchronous dialogs behave nice during shutdown some additional changes were made.

Please note that disabling the tray icon menu when a modal dialog is active is useful itself as on master (4ad59042b359f473d5888ecee0c9288dcf98f1c9) it is possible to switch to the “Receive” tab while the GUI is waiting for a password for the “Send” tab:

Screenshot from 2021-12-17 18-59-51

This is confusing and must be avoided.

Fixes bitcoin/bitcoin#23790.

hebasto added this to the milestone 23.0 on Dec 17, 2021

hebasto added the label Bug on Dec 17, 2021

hebasto added the label Wallet on Dec 17, 2021

ghost commented at 6:29 pm on December 17, 2021: none

I was unable to reproduce the issue mentioned in https://github.com/bitcoin/bitcoin/issues/23790 using PR branch, so issue might be fixed. There was a password prompt each time I tried to send bitcoin from an encrypted wallet.

Would like to test few more things and go through each commit since there are 11 commits in this PR before ACK

willyko commented at 2:18 am on December 18, 2021: contributor

testedACK for the issue mentioned in https://github.com/bitcoin/bitcoin/issues/23790

hebasto force-pushed on Dec 18, 2021

hebasto commented at 6:29 pm on December 18, 2021: member

Updated 702a243d34f8ef1ca1789ee0c9ee70aef7ec821e -> 80e7e303262e149df78cc8f0eb3ce357b03184ca (pr509.01 -> pr509.02, diff).

Dropped QAction::setStatusTip() calls:

status tip works only for top-level parent widget, tray icon’s QMenu instance is not a top-level one
even on master this feature does not work consistently across all supported platforms
the tray icon context menu is not local to the main window status bar, so status tips are likely to be unnoticed by users

ghost commented at 3:26 pm on December 21, 2021: none

~~Is this error related to this PR in any way?~~ UPDATE: Its not related to PR. I tried testing this with 1 commit removed from PR branch in #399 but still got same error so I will create new issue for this.

I get this when I try to load a PSBT for signing. This does not happen in v22.0

PSBT:

0cHNidP8BAFICAAAAAYLWIrucosFvAaD6/JAOgihjyt2xcshd2VdRQd2VUknSAAAAAAD9////ARAnAAAAAAAAFgAUX2caQkxHKburbHQctmMM7btDNunWiiAAAAEA3gIAAAAAAQE6UEKkqOk4pmFFsjgCdJWcr8o/e05sonzbu2r9WuUNIAEAAAAA/f///wINKAAAAAAAABYAFOgtXS/Ov298HhtS87BZe0K/OQ8ooIYBAAAAAAAWABQqd5hFsp4qwKj0vyfuSRN8nFEBoAJHMEQCIBIgXxY+MF/b4MsjbK3D1zE1bcrsIFYNOEA8WmHwAGZeAiBarlG/F19lu3vRY9YznD3rBy6pF5RnjSitGdxUpIml9wEhAhtZklcfD/gpfbAtqeqetj+NSHzadZkQp2+odLAQEp/lp4cgAAAA

unknown approved

unknown commented at 11:52 am on December 24, 2021: none

tACK https://github.com/bitcoin-core/gui/pull/509/commits/80e7e303262e149df78cc8f0eb3ce357b03184ca

This PR fixes the bug reported in https://github.com/bitcoin/bitcoin/issues/23790 and I did not find any issues with the code or testing other things.

ghost commented at 11:57 am on December 24, 2021: none

There is some error in CI though which I missed earlier: https://github.com/bitcoin-core/gui/pull/509/checks?check_run_id=4570631121

hebasto force-pushed on Dec 26, 2021

hebasto commented at 9:13 am on December 26, 2021: member

Updated 80e7e303262e149df78cc8f0eb3ce357b03184ca -> 356e21f68dc953b2efa95fe11954266f655e3eed (pr509.02 -> pr509.03, diff).

Silenced -Wunused-variable warnings for macOS.

unknown approved

unknown commented at 1:01 pm on December 26, 2021: none

reACK https://github.com/bitcoin-core/gui/pull/509/commits/356e21f68dc953b2efa95fe11954266f655e3eed

promag commented at 1:35 am on January 5, 2022: contributor

@hebasto have you tried to fix the regression without reverting the other change?

hebasto commented at 4:09 pm on January 5, 2022: member

@promag

@hebasto have you tried to fix the regression without reverting the other change?

Yes. Although, did not finish it. Changes got more and more evolved.

promag commented at 9:35 am on January 10, 2022: contributor

@hebasto I can see why, there are multiple cases. Not sure how I didn’t catch unlockWallet blocking case on #336#pullrequestreview-766699378 review.

I think the best course of action is to revert and follow up with necessary refactors.

Concept ACK.

w0xlt approved

w0xlt commented at 9:55 am on January 23, 2022: contributor

tACK https://github.com/bitcoin-core/gui/pull/509/commits/356e21f68dc953b2efa95fe11954266f655e3eed

Verified that the error mentioned in https://github.com/bitcoin/bitcoin/issues/23790 happens in master branch and this PR fixes it.

hebasto requested review from achow101 on Jan 24, 2022

in src/qt/bitcoingui.cpp:802 in 7f302e1db6 outdated

788@@ -789,7 +789,11 @@ void BitcoinGUI::createTrayIconMenu()
789         return;
790 
791     trayIcon->setContextMenu(trayIconMenu.get());
792-    connect(trayIcon, &QSystemTrayIcon::activated, this, &BitcoinGUI::trayIconActivated);
793+    connect(trayIcon, &QSystemTrayIcon::activated, [this](QSystemTrayIcon::ActivationReason reason) {
794+        if (reason == QSystemTrayIcon::Trigger) {

Sjors commented at 10:21 am on February 8, 2022:

7f302e1db6da1188bc20c8d27578977531a375cb

0// Click on system tray icon triggers show/hide of the main window

in src/qt/bitcoingui.cpp:777 in e65727fd14 outdated

811-#ifndef Q_OS_MAC
812     // Note: On macOS, the Dock icon's menu already has Show / Hide action.
813     trayIconMenu->addAction(toggleHideAction);
814     trayIconMenu->addSeparator();
815-#endif
816+#endif // Q_OS_MAC

Sjors commented at 10:31 am on February 8, 2022:

e65727fd145968fde3103ad495c658cb5ff9d983 nit: could add blank line here

in src/qt/bitcoingui.cpp:788 in 7b7e6f3348 outdated

784@@ -790,7 +785,7 @@ void BitcoinGUI::createTrayIconMenu()
785     }
786 
787     // Note: On macOS, the Dock icon's menu already has Show / Hide action.
788-    trayIconMenu->addAction(toggleHideAction);
789+    QAction* show_hide_action = trayIconMenu->addAction(tr("Show / &Hide"), this, &BitcoinGUI::toggleHidden);

Sjors commented at 10:36 am on February 8, 2022:

7b7e6f334854f9f193ac9292d7ae82850dbd717c: you lost the tooltip here (though it’s not very useful)

hebasto commented at 4:02 am on February 9, 2022:

Commit message updated.

in src/qt/bitcoingui.cpp:631 in 7b7e6f3348 outdated

623@@ -627,8 +624,6 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
624             trayIcon->setVisible(optionsModel->getShowTrayIcon());
625         }
626     } else {
627-        // Disable possibility to show main window via action
628-        toggleHideAction->setEnabled(false);

Sjors commented at 10:39 am on February 8, 2022:

7b7e6f334854f9f193ac9292d7ae82850dbd717c: why is this safe to drop?

hebasto commented at 2:35 pm on February 8, 2022:

New show_hide_action is onle available via trayIconMenu which is effectively disabled in the next few lines of code.

in src/qt/bitcoingui.cpp:810 in c4e80c5fe5 outdated

806+        [this, show_hide_action, send_action, receive_action] {
807             show_hide_action->setText(
808                 (!isHidden() && !isMinimized() && !GUIUtil::isObscured(this)) ?
809                     tr("&Hide") :
810                     tr("S&how"));
811+            if (enableWallet) {

Sjors commented at 10:49 am on February 8, 2022:

c4e80c5fe5d0c522d107e58b3b9b93a7c973c87c: this breaks behavior, at least on macOS. When I don’t have any wallets loaded, the Send and Receive options are no longer disabled (Sign and Verify are).

Sjors commented at 4:52 pm on February 8, 2022:

It behaves again in 58e16035c1fc513fce0b09e02c7d863c63ec990d, I guess because this code no longer lives inside #ifndef Q_OS_MAC.

in src/qt/bitcoingui.cpp:815 in 91b8f164ad outdated

811                     tr("&Hide") :
812                     tr("S&how"));
813             if (enableWallet) {
814                 send_action->setEnabled(sendCoinsAction->isEnabled());
815                 receive_action->setEnabled(receiveCoinsAction->isEnabled());
816+                sign_action->setEnabled(signMessageAction->isEnabled());

Sjors commented at 10:57 am on February 8, 2022:

91b8f164ad970b5094933a6b9ac379529533c22f: similar to c4e80c5fe5d0c522d107e58b3b9b93a7c973c87c this breaks behavior for the sign and verify buttons when no wallet is loaded.

in src/qt/bitcoingui.cpp:827 in 97692afeb2 outdated

814-                send_action->setEnabled(sendCoinsAction->isEnabled());
815-                receive_action->setEnabled(receiveCoinsAction->isEnabled());
816-                sign_action->setEnabled(signMessageAction->isEnabled());
817-                verify_action->setEnabled(verifyMessageAction->isEnabled());
818+            if (QApplication::activeModalWidget()) {
819+                for (QAction* a : trayIconMenu.get()->actions()) {

Sjors commented at 11:01 am on February 8, 2022:

97692afeb2849b3bd673b2908fd91473b37dbbbb: I don’t see any difference in behavior on macOS; before and after this commit, if I open coin selection, it disables everything except the macOS standard items (Options, Show all windows, Hide, Stop)

in src/qt/bitcoin.cpp:271 in cda881a9ce outdated

264@@ -265,7 +265,11 @@ void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
265     connect(window, &BitcoinGUI::quitRequested, this, &BitcoinApplication::requestShutdown);
266 
267     pollShutdownTimer = new QTimer(window);
268-    connect(pollShutdownTimer, &QTimer::timeout, window, &BitcoinGUI::detectShutdown);
269+    connect(pollShutdownTimer, &QTimer::timeout, [this]{

Sjors commented at 11:06 am on February 8, 2022:

cda881a9ced639d5f22031cf334140a151d9b505 : on macOS when I’m in coin control, before and after this commit, Command + Q is ignored (not delayed). Same with Stop from the tray menu. (that’s fine by the way, a delayed shutdown seems confusing)

But when I hit ctrl + c in the terminal, before this commit it would shut QT down, with this commit it indeed delays the shutdown until the dialog is closed. (that’s also fine, because this method of shutdown is probably only used by developers)

Sjors commented at 11:25 am on February 8, 2022: member

a632303a67e6642aa793af1e74522801b5a12093 indeed fixes the no lock after send regression for me. But it introduces some (much less problematic) regressions, at least on macOS.

qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda c3ca8364b2

qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda 66afa286e5

hebasto force-pushed on Feb 8, 2022

hebasto marked this as a draft on Feb 8, 2022

qt, refactor: Fill up trayIconMenu before connections

This change is required for the following commits.

78189daac8

qt: Drop BitcoinGUI::toggleHideAction member

Also dropped useless tooltip.

ee151d0327

qt: Make show_hide_action dependent on the main window actual state fd667e73cd

qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members 58e16035c1

hebasto force-pushed on Feb 8, 2022

hebasto marked this as ready for review on Feb 8, 2022

in src/qt/bitcoingui.cpp:833 in fd667e73cd outdated

828+        trayIconMenu.get(), &QMenu::aboutToShow,
829+        [this, show_hide_action] {
830+            if (show_hide_action) show_hide_action->setText(
831+                (!isHidden() && !isMinimized() && !GUIUtil::isObscured(this)) ?
832+                    tr("&Hide") :
833+                    tr("S&how"));

Sjors commented at 4:50 pm on February 8, 2022:

fd667e73cd109bbfc14011f8c2c08556648b4c50: you may want to delay this keyboard shortcut, because we’re past the translation cut-off?

Sjors approved

Sjors commented at 5:11 pm on February 8, 2022: member

tACK 74e638afb6e310ef55b07b74fcf388a82030dacc

hebasto commented at 4:10 am on February 9, 2022: member

@Sjors

you may want to delay this keyboard shortcut, because we’re past the translation cut-off?

Change is small, so it’s okay to get it merged before string freeze (now we are in “soft translation string freeze”). @prayank23 @w0xlt @promag

Mind having another look into this PR?

hebasto commented at 5:04 am on February 9, 2022: member

On macOS another behavior change noticed. On master, due to https://github.com/bitcoin-core/gui/blob/b7942c94824e4e06489417ee587d1f3ac631fce0/src/qt/bitcoingui.cpp#L316 optionsAction has forcibly been hidden in the Dock menu (according to design guide, it is moved into application menu):

Screenshot from 2022-02-09 06-49-29

With this PR options_action has no QAction::PreferencesRole set, and it is active:

Screenshot from 2022-02-09 06-56-51 @Sjors What do think about this behavior change?

Sjors commented at 9:39 am on February 9, 2022: member

It seems confusing to have “Options…” and “Options ->” in a single dropdown. I don’t care deeply, but maybe just keep the original behavior?

ghost commented at 3:41 pm on February 10, 2022: none

@prayank23 @w0xlt @promag

Mind having another look into this PR?

I don’t care about UI issues with Mac but this pull request addresses an issue which affects security on every platform, works as expected so I have no issues with it. Would have been better if reverting changes were simple and less line of code in this PR. Hopefully we don’t ship v23.0 with a known issue that affects security.

ACK https://github.com/bitcoin-core/gui/pull/509/commits/74e638afb6e310ef55b07b74fcf388a82030dacc

qt, refactor: Use local QAction instances for the tray icon menu

This change is required for the following commit.

92427354dd

qt: Disable tray icon menu when a modal dialog is active 8c0eb80f41

qt: Delay shutdown while a modal dialog is active 89c277a6fc

qt: Revert 7fa91e831227e556bd8a7ae3da64bd59d4f30d5f partially

The AskPassphraseDialog modal dialog must be synchronous here as
expected in the WalletModel::requestUnlock() function.

Fixed an introduced regression.

5d7666b151

scripted-diff: Rename ShowModalDialogAndDeleteOnClose

-BEGIN VERIFY SCRIPT-
sed -i 's/ShowModalDialogAndDeleteOnClose/ShowModalDialogAsynchronously/' -- $(git grep -l -e "ShowModalDialogAndDeleteOnClose")
-END VERIFY SCRIPT-

It is important to highlight that a modal dialog is showed
asynchronously as there are cases when the synchronous QDialog::exec()
is required.

f730bd7d58

hebasto force-pushed on Feb 12, 2022

hebasto commented at 9:12 pm on February 12, 2022: member

Updated 74e638afb6e310ef55b07b74fcf388a82030dacc -> f730bd7d580502ae3c3b5953ada3724b59f5cd9b (pr509.04 -> pr509.05, diff).

It seems confusing to have “Options…” and “Options ->” in a single dropdown. I don’t care deeply, but maybe just keep the original behavior?

Fixed.

hebasto commented at 7:52 am on February 14, 2022: member

@prayank23 @Sjors

Would you mind re-acking?

unknown approved

unknown commented at 11:28 am on February 14, 2022: none

tACK https://github.com/bitcoin-core/gui/pull/509/commits/f730bd7d580502ae3c3b5953ada3724b59f5cd9b

Tried on Fedora and found no issues.

hebasto requested review from jarolrod on Feb 14, 2022

hebasto merged this on Feb 15, 2022

hebasto closed this on Feb 15, 2022

hebasto deleted the branch on Feb 15, 2022

Sjors commented at 10:21 am on February 15, 2022: member

Post merge tACK, only difference is the PreferencesRole and the duplicate Options is gone.

jarolrod commented at 11:58 am on February 15, 2022: member

Post merge tACK, I was able to reproduce the original issue and I’ve done testing to ensure that this PR fixes this.

sidhujag referenced this in commit 1aaffec63c on Feb 15, 2022

laanwj referenced this in commit c44e734dca on Feb 22, 2022

Sjors commented at 11:12 am on March 29, 2022: member

I wonder if this was also a problem when creating a wallet, see #571.

bitcoin-core locked this on Mar 29, 2023

Respect dialog modality and fix a regression in wallet unlock #509