Restore Send button when using external signer

Sjors commented at 12:54 pm on February 21, 2022: member

Fixes #551

For the simplest use case of a wallet with one external signer and “PSBT Controls” disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there’s no “Create Unsigned” button.

When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction.

In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before #441).

This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review.

hebasto commented at 1:27 pm on February 21, 2022: member

Concept ACK.

I’ve verified that no new or changed strings are presented.

hebasto added this to the milestone 23.0 on Feb 21, 2022

hebasto added the label UX on Feb 21, 2022

hebasto added the label Wallet on Feb 21, 2022

Sjors requested review from achow101 on Feb 21, 2022

in src/qt/sendcoinsdialog.cpp:404 in 5e6cda4b3b outdated

400@@ -401,6 +401,52 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa
401     return true;
402 }
403 
404+void SendCoinsDialog::presentPSBT(PartiallySignedTransaction &psbtx)

promag commented at 9:41 pm on February 22, 2022:

5e6cda4b3b5707b246d36406578c92cb20f47cb7

nit, & before space 🙉 header too

in src/qt/sendcoinsdialog.cpp:512 in a49bb0c832 outdated

508         PartiallySignedTransaction psbtx(mtx);
509+        // Copy PSBT to clipboard and offer to save
510+        presentPSBT(psbtx);
511+    } else { // Send clicked
512+        assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner());
513         bool complete = false;

promag commented at 10:01 pm on February 22, 2022:

a49bb0c8328d1e0ecf0ee80916594ff260f79613

I think it makes more sense to refactor this to:

 0bool broadcast = true;
 1if (model->wallet().hasExternalSigner()) {
 2    // ...
 3    bool complete;
 4    send_failure = !signWithExternalSigner(psbtx, mtx, complete);
 5    if (complete) {
 6        // ...
 7    } else {
 8        // ...
 9        broadcast = false
10    }
11}
12if (broadcast) {
13    // ...
14}

promag commented at 10:01 pm on February 22, 2022: contributor

Concept ACK

move-only: helper function to present PSBT

This commit does not change behavior.

Review hint:
git show --color-moved --color-moved-ws=allow-indentation-change

026b5b4523

Sjors force-pushed on Feb 24, 2022

Sjors commented at 11:42 am on February 24, 2022: member

Addressed @promag’s feedback.

DrahtBot commented at 5:49 am on March 2, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#562 (Warn when sending to already-used Bitcoin addresses by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 10:02 pm on March 6, 2022: contributor

@Sjors it will broadcast even if you reject signing on the device, so it’s missing broadcast = false in that case.

in src/qt/sendcoinsdialog.cpp:505 in 33d165d802 outdated

501@@ -502,19 +502,34 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
502     }
503 
504     bool send_failure = false;
505-    if (retval == QMessageBox::Save) {
506+    bool broadcast = true;

promag commented at 11:09 pm on March 6, 2022:

33d165d802846943c0ba70be4e643e3b50c6634e

 0diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
 1index c420aa3581..87a0e4a098 100644
 2--- a/src/qt/sendcoinsdialog.cpp
 3+++ b/src/qt/sendcoinsdialog.cpp
 4@@ -502,7 +502,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 5     }
 6
 7     bool send_failure = false;
 8-    bool broadcast = true;
 9     if (retval == QMessageBox::Save) { // Create Unsigned clicked
10         CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
11         PartiallySignedTransaction psbtx(mtx);
12@@ -510,11 +509,13 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
13         presentPSBT(psbtx);
14     } else { // Send clicked
15         assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner());
16+        bool broadcast = true;
17         if (model->wallet().hasExternalSigner()) {
18             CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
19             PartiallySignedTransaction psbtx(mtx);
20             bool complete = false;
21             send_failure = !signWithExternalSigner(psbtx, mtx, complete);
22+            broadcast = complete;
23             // A transaction signed with an external signer is not always complete,
24             // e.g. in a multisig wallet.
25             if (complete) {
26@@ -523,7 +524,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
27                 m_current_transaction->setWtx(tx);
28             } else if (!send_failure) {
29                 presentPSBT(psbtx);
30-                broadcast = false;
31             }
32         }

Sjors commented at 7:03 pm on March 10, 2022:

If the user rejects the transaction then complete = false and it will present the PSBT. We can’t tell the difference between rejection and failure, so that’s difficult to avoid. I don’t see how this patch would change that.

promag commented at 7:15 pm on March 10, 2022:

If user rejects then complete = false and send_failure = true, so it doesn’t presentPSBT() and because broadcast = true it calls model->sendCoins()

Sjors commented at 9:20 pm on March 10, 2022:

I see. I restructured this code and fix a bit…

promag commented at 9:30 pm on March 10, 2022:

Thanks, LGTM, will re-test.

Sjors renamed this:
~~Enable Send button when using external signer~~
Restore Send button when using external signer
on Mar 10, 2022

jonatack commented at 7:08 pm on March 10, 2022: contributor

Concept ACK

Sjors force-pushed on Mar 10, 2022

kristapsk commented at 9:27 pm on March 10, 2022: contributor

Concept ACK

promag approved

promag commented at 1:31 am on March 11, 2022: contributor

Tested ACK 86e3e2cda8f2ed91f13ea0cf7c0bf291083c1970.

in src/qt/sendcoinsdialog.h:86 in 86e3e2cda8 outdated

81     bool PrepareSendText(QString& question_string, QString& informative_text, QString& detailed_text);
82+    /* Sign PSBT using external signer.
83+     *
84+     * @param psbtx the PSBT to sign
85+     * @param mtx needed to attempt to finalize
86+     * @param complete whether the PSBT is complete (a succesfully signed multisig transaction may not be complete)

jonatack commented at 9:43 am on March 11, 2022:

e1c1c615 Thanks for adding the doxygen documentation.

These 3 @param might be more explicit as @param[in,out]

0     * [@param](/bitcoin-core-gui/contributor/param/) complete whether the PSBT is complete (a successfully signed multisig transaction may not be complete)

in src/qt/sendcoinsdialog.cpp:453 in 86e3e2cda8 outdated

448+}
449+
450+bool SendCoinsDialog::signWithExternalSigner(PartiallySignedTransaction& psbtx, CMutableTransaction& mtx, bool& complete) {
451+    // Always fill without signing first. This prevents an external signer
452+    // from being called prematurely and is not expensive.
453+    TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete);

jonatack commented at 9:48 am on March 11, 2022:

e1c1c615105ce7e2bf76b1e9522408e369ff0c5f if you retouch, clang-tidy named args format (and maybe add for n_signed)

0    TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);

in src/qt/sendcoinsdialog.h:88 in 86e3e2cda8 outdated

83+     *
84+     * @param psbtx the PSBT to sign
85+     * @param mtx needed to attempt to finalize
86+     * @param complete whether the PSBT is complete (a succesfully signed multisig transaction may not be complete)
87+     *
88+     * @returns false if any failure occured, which may include the user rejection a transaction on the device.

jonatack commented at 9:52 am on March 11, 2022:

e1c1c615105ce7e2bf76b1e9522408e369ff0c5f s/occured/occurred/ and missing the word “of” if I understand the intended message

0     * [@returns](/bitcoin-core-gui/contributor/returns/) false if any failure occurred, which may include the user rejection of a transaction on the device.

in src/qt/sendcoinsdialog.cpp:493 in 86e3e2cda8 outdated

489@@ -411,7 +490,7 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
490     assert(m_current_transaction);
491 
492     const QString confirmation = tr("Confirm send coins");
493-    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, !model->wallet().privateKeysDisabled(), model->getOptionsModel()->getEnablePSBTControls(), this);
494+    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, /*enable_send=*/!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner(), /*always_show_unsigned=*/model->getOptionsModel()->getEnablePSBTControls(), this);

jonatack commented at 10:14 am on March 11, 2022:

33d165d readability suggestion, replace named args with localvars of same name here

0+    const bool enable_send{!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner()};
1+    const bool always_show_unsigned{model->getOptionsModel()->getEnablePSBTControls()};

0    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, enable_send, always_show_unsigned, this);

in src/qt/sendcoinsdialog.cpp:510 in 86e3e2cda8 outdated

512-        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete);
513-        assert(!complete);
514-        assert(err == TransactionError::OK);
515+        // Copy PSBT to clipboard and offer to save
516+        presentPSBT(psbtx);
517+    } else { // Send clicked

jonatack commented at 10:17 am on March 11, 2022:

33d165d readability suggestion

 0     bool send_failure = false;
 1-    if (retval == QMessageBox::Save) { // Create Unsigned clicked
 2+    if (retval == QMessageBox::Save) {
 3+        // "Create Unsigned" clicked
 4         CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
 5         PartiallySignedTransaction psbtx(mtx);
 6         // Copy PSBT to clipboard and offer to save
 7         presentPSBT(psbtx);
 8-    } else { // Send clicked
 9+    } else {
10+        // "Send" clicked
11         assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner());

in src/qt/sendcoinsdialog.h:132 in 86e3e2cda8 outdated

127@@ -117,6 +128,8 @@ class SendConfirmationDialog : public QMessageBox
128 
129 public:
130     SendConfirmationDialog(const QString& title, const QString& text, const QString& informative_text = "", const QString& detailed_text = "", int secDelay = SEND_CONFIRM_DELAY, bool enable_send = true, bool always_show_unsigned = true, QWidget* parent = nullptr);
131+    /* Returns QMessageBox::Cancel, QMessageBox::Yes when send is
132+       clicked and QMessageBox::Save when Create Unsigned is clicked. */

jonatack commented at 10:24 am on March 11, 2022:

33d165d maybe s/send/Send/, and wrap Send and Create Unsigned in quotes

jonatack commented at 10:32 am on March 11, 2022: contributor

Untested ACK 86e3e2cda8f2ed91f13ea0cf7c0bf291083c1970 code review and debug build, the approach and logic looks correct

A few style and readability suggestions, happy to re-review if you update.

in src/qt/sendcoinsdialog.cpp:432 in 86e3e2cda8 outdated

507         CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
508         PartiallySignedTransaction psbtx(mtx);
509-        bool complete = false;
510-        // Always fill without signing first. This prevents an external signer
511-        // from being called prematurely and is not expensive.
512-        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete);

luke-jr commented at 9:58 pm on March 14, 2022:

e1c1c615105ce7e2bf76b1e9522408e369ff0c5f claims “Does not change behavior.”, yet this is no longer done for wallets without an external signer.

Sjors commented at 3:42 pm on March 15, 2022:

Good point. I moved this out of the helper function.

It leads to some duplication in 053861480417ad48aa37e0376eedbea629e052b7, but it’s probably better to preserve the behavior. It might even useful, though I’m a bit rusty on what PSBT filling does on top of what the wallet already does.

jonatack commented at 7:37 pm on March 16, 2022:

Yes, as a potential follow-up it looks like this code block could be potentially de-duped by extracting to a helper method that returns psbtx and mtx.

0        CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
1        PartiallySignedTransaction psbtx(mtx);
2        bool complete = false;
3        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);
4        assert(!complete);
5        assert(err == TransactionError::OK);

(and TransactionError err could be const)

Sjors commented at 7:57 pm on March 16, 2022:

I’m going to avoid touching this now, but I agree it would be good to move into a helper.

luke-jr commented at 10:15 pm on March 14, 2022: member

utACK 86e3e2cda8f2ed91f13ea0cf7c0bf291083c1970 except for 1 part I’m unsure on

Sjors force-pushed on Mar 15, 2022

Sjors commented at 3:42 pm on March 15, 2022: member

Addressed @jonatack’s feedback, since I had to touch it for the issue @luke-jr raised.

Sjors force-pushed on Mar 15, 2022

in src/qt/sendcoinsdialog.cpp:453 in 0538614804 outdated

448+}
449+
450+bool SendCoinsDialog::signWithExternalSigner(PartiallySignedTransaction& psbtx, CMutableTransaction& mtx, bool& complete) {
451+    TransactionError err;
452+    try {
453+        err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);

luke-jr commented at 11:17 pm on March 15, 2022:

sign was true here - why was it changed to false?

Sjors commented at 9:29 am on March 16, 2022:

Yikes, I broke that during the style fix. (fixed and re-tested on device this time)

luke-jr changes_requested

refactor: helper function signWithExternalSigner()

Does not change behavior.

Review hint:
git show --color-moved --color-moved-ws=allow-indentation-change

4b5a6cd149

gui: restore Send for external signer

Before this change the send confirmation dialog would keep the Send option disabled. The Create Unsigned choice would actually send. This is potentially confusing.

With this change the Create Unsigned button will not attempt to sign and always produce a PSBT. The Send button will attempt to sign, and only return a PSBT if more signatures are needed.

When using an external signer, the Create Unsigned option only appears when PSBT controls are enabled in the wallet settings.

This commit maintains the pre-existing behavior of filling the PSBT (without signing) even when not using an external signer.

Closes #551

Co-authored-by: Jon Atack <jon@atack.com>

2efdfb88aa

Sjors force-pushed on Mar 16, 2022

luke-jr approved

luke-jr commented at 4:54 pm on March 16, 2022: member

utACK 2efdfb88aab6496dcf2b98e0de30635bc6bade85

jonatack commented at 7:53 pm on March 16, 2022: contributor

utACK 2efdfb88aab6496dcf2b98e0de30635bc6bade85 diff review since my last review, code re-review, rebased to current master, verified clean debug build of each commit

hebasto added the label Needs backport (23.x) on Mar 17, 2022

hebasto merged this on Mar 17, 2022

hebasto closed this on Mar 17, 2022

hebasto referenced this in commit fc421d4c8c on Mar 17, 2022

hebasto referenced this in commit 940694664d on Mar 17, 2022

hebasto referenced this in commit 642f2726de on Mar 17, 2022

hebasto commented at 6:35 am on March 17, 2022: member

Backported in https://github.com/bitcoin/bitcoin/pull/24596.

hebasto removed the label Needs backport (23.x) on Mar 17, 2022

jonatack commented at 8:49 am on March 17, 2022: contributor

It might be good to have another tested ACK of the latest push.

Sjors commented at 9:31 am on March 17, 2022: member

@achow101?

sidhujag referenced this in commit e19f98b3d3 on Mar 18, 2022

MarcoFalke referenced this in commit 7d03cf632d on Mar 24, 2022

fujicoin referenced this in commit 85b3b04392 on Mar 25, 2022

fujicoin referenced this in commit 7866b8850a on Mar 25, 2022

fujicoin referenced this in commit 47c94164ad on Mar 25, 2022

backpacker69 referenced this in commit a3ffd89267 on Jan 18, 2023

backpacker69 referenced this in commit 078cd1ce30 on Jan 18, 2023

backpacker69 referenced this in commit a77578477f on Jan 18, 2023

bitcoin-core locked this on Jun 5, 2023

Restore Send button when using external signer #555

Conflicts