Fix "Load PSBT" functionality when no wallet loaded #399

psancheti110 · 2021-08-07T19:26:23Z

This PR provides a fix to the issue mentioned in #232.

Currently, the Load PSBT functionality works well in case a wallet is loaded but does nothing when a wallet isn't loaded.

If a function cannot work without a wallet being loaded, it is disabled by default (It is unclickable as shown in the image).

For e.g. One cannot Close Wallet or Backup Wallet or Sign Messages without a wallet being loaded. And hence they are disabled. But if you notice, Load PSBT options are not disabled by default even when a wallet isn't loaded.

As mentioned by hebasto in the issue description :

<hebasto> achow101: does "File" -> "Load PSBT from {file|clipboard}" make any sense when no wallet is loaded?
<achow101> hebasto: yes, for finalize and sending

This means Load PSBT should be working just as similar whether wallets are being loaded or not.

After making the required changes to the code, The Load PSBT works as expected even with no wallet loaded and the PSBT is finalized.

Master	PR

Close #232

hebasto · 2021-08-07T19:29:32Z

Concept ACK.

achow101

Concept ACK

src/qt/psbtoperationsdialog.cpp

src/qt/walletframe.cpp

psancheti110 · 2021-08-10T14:13:16Z

PR Updated from a77d04f -> cb7b222. Click here to check diff.

Addressed changes suggested by @achow101 in #399 (review).

hebasto

Approach ACK cb7b222.

After moving PSBT dialog code from walletview.cpp into waletframe.cpp, some includes could be removed:

--- a/src/qt/walletview.cpp
+++ b/src/qt/walletview.cpp
@@ -8,7 +8,6 @@
 #include <qt/askpassphrasedialog.h>
 #include <qt/clientmodel.h>
 #include <qt/guiutil.h>
-#include <qt/psbtoperationsdialog.h>
 #include <qt/optionsmodel.h>
 #include <qt/overviewpage.h>
 #include <qt/platformstyle.h>
@@ -21,13 +20,10 @@
 
 #include <interfaces/node.h>
 #include <node/ui_interface.h>
-#include <psbt.h>
 #include <util/strencodings.h>
 
 #include <QAction>
 #include <QActionGroup>
-#include <QApplication>
-#include <QClipboard>
 #include <QFileDialog>
 #include <QHBoxLayout>
 #include <QProgressDialog>

src/qt/psbtoperationsdialog.cpp

hebasto · 2021-08-10T14:53:28Z

src/qt/walletframe.cpp

+        Q_EMIT message(tr("Error"), tr("Unable to decode PSBT") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
+        return;
    }
+    PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);


nit: Add an empty line above as it was in walletview.cpp for readability and easier verifying with diff --color-moved=dimmed-zebra?

hebasto · 2021-08-10T14:58:03Z

src/qt/walletframe.cpp

-    if (walletView) {
-        walletView->gotoLoadPSBT(from_clipboard);
+    std::string data;
+    if (from_clipboard) {


nit: Add an empty line above as it was in walletview.cpp for readability and easier verifying with diff --color-moved=dimmed-zebra?

hebasto · 2021-08-10T15:07:12Z

src/qt/walletframe.cpp

+        QString filename = GUIUtil::getOpenFileName(this,
+                                                    tr("Load Transaction Data"), QString(),
+                                                    tr("Partially Signed Transaction (*.psbt)"), nullptr);


I understand these indentations were suggested by clang-format-diff.py, but to keep verifying easy with diff --color-moved=dimmed-zebra, I'd suggest to keep the same indentations as they were in walletview.cpp.

psancheti110 · 2021-08-10T19:00:06Z

PR Updated from cb7b222 -> e5e0091. Click here to check diff.

Addressed changes suggested by @hebasto in #399 (review)

achow101 · 2021-08-10T20:16:52Z

ACK e5e0091

hebasto

Tested e5e0091.

I think the PSBTOperationsDialog::showTransactionStatus should be modified to correctly handle unsigned PSBTs:

--- a/src/qt/psbtoperationsdialog.cpp
+++ b/src/qt/psbtoperationsdialog.cpp
@@ -254,7 +254,10 @@ void PSBTOperationsDialog::showTransactionStatus(const PartiallySignedTransactio
         case PSBTRole::SIGNER: {
             QString need_sig_text = tr("Transaction still needs signature(s).");
             StatusLevel level = StatusLevel::INFO;
-            if (m_wallet_model->wallet().privateKeysDisabled()) {
+            if (!m_wallet_model) {
+                need_sig_text += " " + tr("(But no wallet is loaded.)");
+                level = StatusLevel::WARN;
+            } else if (m_wallet_model->wallet().privateKeysDisabled()) {
                 need_sig_text += " " + tr("(But this wallet cannot sign transactions.)");
                 level = StatusLevel::WARN;
             } else if (n_could_sign < 1) {

psancheti110 · 2021-08-11T10:14:16Z

PR Updated from e5e0091 -> 0237d95. Click here to check diff.

Addressed changes suggested by @hebasto in #399 (review)

hebasto

ACK 0237d95, tested on Linux Mint 20.2 (Qt 5.12.8).

No wallet is loaded when testing: bitcoin-qt -nowallet.

A. The loading of an unsigned PSBT being prepared with the walletcreatefundedpsbt RPC command:

B. The loading of a signed PSBT which is the PSBT mentioned above being processed with the walletprocesspsbt RPC command:

achow101 · 2021-08-11T19:14:43Z

re-ACK 0237d95

psancheti110 · 2021-08-12T17:54:42Z

A big thanks to all the contributors who took out time to test and review my PR.
Appreciate each and every comment made here by fellow mates 🙌🏻

hebasto added Bug Something isn't working UX All about "how to get things done" Wallet labels Aug 7, 2021

psancheti110 changed the title ~~[WIP] qt: Fix "Load PSBT" functionality when no wallet loaded~~ qt: Fix "Load PSBT" functionality when no wallet loaded Aug 7, 2021

psancheti110 marked this pull request as ready for review August 7, 2021 19:36

hebasto changed the title ~~qt: Fix "Load PSBT" functionality when no wallet loaded~~ Fix "Load PSBT" functionality when no wallet loaded Aug 7, 2021

achow101 reviewed Aug 9, 2021

View reviewed changes

src/qt/psbtoperationsdialog.cpp Show resolved Hide resolved

src/qt/psbtoperationsdialog.cpp Outdated Show resolved Hide resolved

src/qt/psbtoperationsdialog.cpp Show resolved Hide resolved

src/qt/psbtoperationsdialog.cpp Outdated Show resolved Hide resolved

hebasto reviewed Aug 10, 2021

View reviewed changes

src/qt/walletframe.cpp Outdated Show resolved Hide resolved

src/qt/walletframe.cpp Outdated Show resolved Hide resolved

psancheti110 force-pushed the i232-0806 branch from a77d04f to cb7b222 Compare August 10, 2021 10:36

hebasto reviewed Aug 10, 2021

View reviewed changes

psancheti110 force-pushed the i232-0806 branch from cb7b222 to e5e0091 Compare August 10, 2021 18:57

hebasto reviewed Aug 11, 2021

View reviewed changes

qt: Add Load PSBT functionaliy with nowallet

0237d95

psancheti110 force-pushed the i232-0806 branch from e5e0091 to 0237d95 Compare August 11, 2021 10:11

hebasto approved these changes Aug 11, 2021

View reviewed changes

hebasto merged commit e614cc8 into bitcoin-core:master Aug 11, 2021

This was referenced Aug 14, 2021

refactor: Make paths to update Encryption and HD wallet statuses simpler #403

Merged

Do not exit and re-enter main event loop during shutdown #336

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 15, 2021

erge bitcoin-core/gui#399: Fix "Load PSBT" functionality when no wal…

32e3d70

ghost mentioned this pull request Dec 24, 2021

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

Merged

hebasto mentioned this pull request Dec 25, 2021

Unable to load PSBT #511

Open

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Fix "Load PSBT" functionality when no wallet loaded #399

Fix "Load PSBT" functionality when no wallet loaded #399

Uh oh!

Conversation

psancheti110 commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psancheti110 commented Aug 10, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

psancheti110 commented Aug 10, 2021

Uh oh!

achow101 commented Aug 10, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

psancheti110 commented Aug 11, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 11, 2021

Uh oh!

psancheti110 commented Aug 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

psancheti110 commented Aug 7, 2021 •

edited

Loading

hebasto commented Aug 7, 2021 •

edited

Loading