Fix "Load PSBT" functionality when no wallet loaded

psancheti110 commented at 7:26 pm on August 7, 2021: contributor

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 :

0<hebasto> achow101: does "File" -> "Load PSBT from {file|clipboard}" make any sense when no wallet is loaded?
1<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 commented at 7:29 pm on August 7, 2021: member

Concept ACK.

hebasto added the label Bug on Aug 7, 2021

hebasto added the label UX on Aug 7, 2021

hebasto added the label Wallet on Aug 7, 2021

psancheti110 renamed this:
~~[WIP] qt: Fix "Load PSBT" functionality when no wallet loaded~~
qt: Fix "Load PSBT" functionality when no wallet loaded
on Aug 7, 2021

psancheti110 marked this as ready for review on Aug 7, 2021

hebasto renamed this:
~~qt: Fix "Load PSBT" functionality when no wallet loaded~~
Fix "Load PSBT" functionality when no wallet loaded
on Aug 7, 2021

in src/qt/psbtoperationsdialog.cpp:240 in a77d04fb1a outdated

243@@ -237,7 +244,6 @@ size_t PSBTOperationsDialog::couldSignInputs(const PartiallySignedTransaction &p
244 void PSBTOperationsDialog::showTransactionStatus(const PartiallySignedTransaction &psbtx) {
245     PSBTAnalysis analysis = AnalyzePSBT(psbtx);
246     size_t n_could_sign = couldSignInputs(psbtx);
247-

achow101 commented at 6:51 pm on August 9, 2021:

nit: Unnecessary whitespace change.

in src/qt/psbtoperationsdialog.cpp:52 in a77d04fb1a outdated

56-            .arg(QString::fromStdString(TransactionErrorString(err).translated)), StatusLevel::ERR);
57-        return;
58+    if (m_wallet_model) {
59+        bool complete;
60+        size_t n_could_sign;
61+        FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness.

achow101 commented at 6:52 pm on August 9, 2021:

FinalizePSBT is not a wallet function, it should always be done. It also returns a bool indicating whether the transaction is completely signed, so this could be

bool complete = FinalizePSBT(psbt);

in src/qt/psbtoperationsdialog.cpp:60 in a77d04fb1a outdated

71+    } else {
72+        m_ui->broadcastTransactionButton->setEnabled(true);
73+        m_ui->signTransactionButton->setEnabled(false);
74     }
75-
76-    m_ui->broadcastTransactionButton->setEnabled(complete);

achow101 commented at 6:53 pm on August 9, 2021:

If complete is set by FinalizePSBT as I suggested above, then this line can be unchanged and the broadcastTransactionButton lines can be removed from the if-else blocks.

in src/qt/psbtoperationsdialog.cpp:63 in a77d04fb1a outdated

67+            return;
68+        }
69+        m_ui->broadcastTransactionButton->setEnabled(complete);
70+        m_ui->signTransactionButton->setEnabled(!complete && !m_wallet_model->wallet().privateKeysDisabled() && n_could_sign > 0);
71+    } else {
72+        m_ui->broadcastTransactionButton->setEnabled(true);

achow101 commented at 6:54 pm on August 9, 2021:

It’s not always correct for the broadcast button to be enabled since the user could provide a PSBT that is incomplete. By always doing FinalizePSBT as I have suggested above, then we will know whether the PSBT is complete by the time we get here.

achow101 commented at 6:55 pm on August 9, 2021: member

Concept ACK

in src/qt/walletframe.cpp:22 in a77d04fb1a outdated

23+#include <qt/walletmodel.h>
24+#include <qt/walletview.h>
25+
26+#include <node/ui_interface.h>
27+#include <psbt.h>
28+#include <qt/psbtoperationsdialog.h>

hebasto commented at 7:32 am on August 10, 2021:

Please keep the grouping of headers unchanged – (1) our own headers, (2) C++ headers, (3) Qt headers.

in src/qt/walletframe.cpp:23 in a77d04fb1a outdated

24+#include <qt/walletview.h>
25+
26+#include <node/ui_interface.h>
27+#include <psbt.h>
28+#include <qt/psbtoperationsdialog.h>
29+

hebasto commented at 7:33 am on August 10, 2021:

Why this line added?

psancheti110 force-pushed on Aug 10, 2021

psancheti110 commented at 2:13 pm on August 10, 2021: contributor

PR Updated from a77d04fb1a4e788b51424e8fb41122a0568e2756 -> cb7b222cb2ccd1b7d92f7123557097559b0c16a1. Click here to check diff.

Addressed changes suggested by @achow101 in #399#pullrequestreview-725704910.

in src/qt/psbtoperationsdialog.cpp:50 in cb7b222cb2 outdated

53-    TransactionError err = m_wallet_model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, &n_could_sign, m_transaction_data, complete);
54-    if (err != TransactionError::OK) {
55-        showStatus(tr("Failed to load transaction: %1")
56-            .arg(QString::fromStdString(TransactionErrorString(err).translated)), StatusLevel::ERR);
57-        return;
58+    bool complete = FinalizePSBT(psbtx); // Make sure all existing signatures are fully combined before checking for completeness.;

hebasto commented at 2:51 pm on August 10, 2021:

nit: extra ; at the comment end?

in src/qt/walletframe.cpp:223 in cb7b222cb2 outdated

219+    PartiallySignedTransaction psbtx;
220+    if (!DecodeRawPSBT(psbtx, data, error)) {
221+        Q_EMIT message(tr("Error"), tr("Unable to decode PSBT") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
222+        return;
223     }
224+    PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);

hebasto commented at 2:53 pm on August 10, 2021:

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

in src/qt/walletframe.cpp:195 in cb7b222cb2 outdated

189@@ -184,10 +190,38 @@ void WalletFrame::gotoVerifyMessageTab(QString addr)
190 
191 void WalletFrame::gotoLoadPSBT(bool from_clipboard)
192 {
193-    WalletView *walletView = currentWalletView();
194-    if (walletView) {
195-        walletView->gotoLoadPSBT(from_clipboard);
196+    std::string data;
197+    if (from_clipboard) {

hebasto commented at 2:58 pm on August 10, 2021:

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

in src/qt/walletframe.cpp:205 in cb7b222cb2 outdated

203+            return;
204+        }
205+    } else {
206+        QString filename = GUIUtil::getOpenFileName(this,
207+                                                    tr("Load Transaction Data"), QString(),
208+                                                    tr("Partially Signed Transaction (*.psbt)"), nullptr);

hebasto commented at 3:07 pm on August 10, 2021:

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.

hebasto commented at 3:14 pm on August 10, 2021: member

Approach ACK cb7b222cb2ccd1b7d92f7123557097559b0c16a1.

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

 0--- a/src/qt/walletview.cpp
 1+++ b/src/qt/walletview.cpp
 2@@ -8,7 +8,6 @@
 3 #include <qt/askpassphrasedialog.h>
 4 #include <qt/clientmodel.h>
 5 #include <qt/guiutil.h>
 6-#include <qt/psbtoperationsdialog.h>
 7 #include <qt/optionsmodel.h>
 8 #include <qt/overviewpage.h>
 9 #include <qt/platformstyle.h>
10@@ -21,13 +20,10 @@
11 
12 #include <interfaces/node.h>
13 #include <node/ui_interface.h>
14-#include <psbt.h>
15 #include <util/strencodings.h>
16 
17 #include <QAction>
18 #include <QActionGroup>
19-#include <QApplication>
20-#include <QClipboard>
21 #include <QFileDialog>
22 #include <QHBoxLayout>
23 #include <QProgressDialog>

psancheti110 force-pushed on Aug 10, 2021

psancheti110 commented at 7:00 pm on August 10, 2021: contributor

PR Updated from cb7b222cb2ccd1b7d92f7123557097559b0c16a1 -> e5e00910ae5fc3d07e1a4ed5aff5b23a26166495. Click here to check diff.

Addressed changes suggested by @hebasto in #399#pullrequestreview-726541283

achow101 commented at 8:16 pm on August 10, 2021: member

ACK e5e00910ae5fc3d07e1a4ed5aff5b23a26166495

hebasto commented at 7:46 am on August 11, 2021: member

Tested e5e00910ae5fc3d07e1a4ed5aff5b23a26166495.

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

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

qt: Add Load PSBT functionaliy with nowallet 0237d95323

psancheti110 force-pushed on Aug 11, 2021

psancheti110 commented at 10:14 am on August 11, 2021: contributor

PR Updated from e5e00910ae5fc3d07e1a4ed5aff5b23a26166495 -> 0237d95323dec7c65b7223c314afdf63aab6b11b. Click here to check diff.

Addressed changes suggested by @hebasto in #399#pullrequestreview-727158375

hebasto approved

hebasto commented at 11:09 am on August 11, 2021: member

ACK 0237d95323dec7c65b7223c314afdf63aab6b11b, 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:

Screenshot from 2021-08-11 14-06-59

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

Screenshot from 2021-08-11 14-07-33

achow101 commented at 7:14 pm on August 11, 2021: member

re-ACK 0237d95323dec7c65b7223c314afdf63aab6b11b

hebasto merged this on Aug 11, 2021

hebasto closed this on Aug 11, 2021

psancheti110 commented at 5:54 pm on August 12, 2021: contributor

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 🙌🏻

sidhujag referenced this in commit 32e3d70310 on Aug 15, 2021

bitcoin-core locked this on Aug 16, 2022

Fix “Load PSBT” functionality when no wallet loaded #399