Restore Send button when using external signer #555

pull Sjors wants to merge 3 commits into bitcoin-core:master from Sjors:2022/02/send_button changing 2 files +125 −93
  1. 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.

  2. hebasto commented at 1:27 PM on February 21, 2022: member

    Concept ACK.

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

  3. hebasto added this to the milestone 23.0 on Feb 21, 2022
  4. hebasto added the label UX on Feb 21, 2022
  5. hebasto added the label Wallet on Feb 21, 2022
  6. Sjors requested review from achow101 on Feb 21, 2022
  7. 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

  8. 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:

    bool broadcast = true;
if (model->wallet().hasExternalSigner()) {
    // ...
    bool complete;
    send_failure = !signWithExternalSigner(psbtx, mtx, complete);
    if (complete) {
        // ...
    } else {
        // ...
        broadcast = false
    }
}
if (broadcast) {
    // ...
}
  9. promag commented at 10:01 PM on February 22, 2022: contributor

    Concept ACK

  10. 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
  11. Sjors force-pushed on Feb 24, 2022
  12. Sjors commented at 11:42 AM on February 24, 2022: member

    Addressed @promag's feedback.

  13. DrahtBot commented at 5:49 AM on March 2, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

    diff --git a/src/qt/sendcoinsdialog.cpp b/src/qt/sendcoinsdialog.cpp
index c420aa3581..87a0e4a098 100644
--- a/src/qt/sendcoinsdialog.cpp
+++ b/src/qt/sendcoinsdialog.cpp
@@ -502,7 +502,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
     }

     bool send_failure = false;
-    bool broadcast = true;
     if (retval == QMessageBox::Save) { // Create Unsigned clicked
         CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
         PartiallySignedTransaction psbtx(mtx);
@@ -510,11 +509,13 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
         presentPSBT(psbtx);
     } else { // Send clicked
         assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner());
+        bool broadcast = true;
         if (model->wallet().hasExternalSigner()) {
             CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
             PartiallySignedTransaction psbtx(mtx);
             bool complete = false;
             send_failure = !signWithExternalSigner(psbtx, mtx, complete);
+            broadcast = complete;
             // A transaction signed with an external signer is not always complete,
             // e.g. in a multisig wallet.
             if (complete) {
@@ -523,7 +524,6 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
                 m_current_transaction->setWtx(tx);
             } else if (!send_failure) {
                 presentPSBT(psbtx);
-                broadcast = false;
             }
         }
    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.

  16. Sjors renamed this:
    Enable Send button when using external signer
    Restore Send button when using external signer
    on Mar 10, 2022
  17. jonatack commented at 7:08 PM on March 10, 2022: contributor

    Concept ACK

  18. Sjors force-pushed on Mar 10, 2022
  19. Sjors force-pushed on Mar 10, 2022
  20. kristapsk commented at 9:27 PM on March 10, 2022: contributor

    Concept ACK

  21. promag approved
  22. promag commented at 1:31 AM on March 11, 2022: contributor

    Tested ACK 86e3e2cda8f2ed91f13ea0cf7c0bf291083c1970.

  23. 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]

         * [@param](/bitcoin-core-gui/contributor/param/) complete whether the PSBT is complete (a successfully signed multisig transaction may not be complete)
  24. 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)

        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);
  25. 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

         * [@returns](/bitcoin-core-gui/contributor/returns/) false if any failure occurred, which may include the user rejection of a transaction on the device.
  26. 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

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

        auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, enable_send, always_show_unsigned, this);
  27. 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

         bool send_failure = false;
-    if (retval == QMessageBox::Save) { // Create Unsigned clicked
+    if (retval == QMessageBox::Save) {
+        // "Create Unsigned" clicked
         CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
         PartiallySignedTransaction psbtx(mtx);
         // Copy PSBT to clipboard and offer to save
         presentPSBT(psbtx);
-    } else { // Send clicked
+    } else {
+        // "Send" clicked
         assert(!model->wallet().privateKeysDisabled() || model->wallet().hasExternalSigner());
  28. 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

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

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

            CMutableTransaction mtx = CMutableTransaction{*(m_current_transaction->getWtx())};
        PartiallySignedTransaction psbtx(mtx);
        bool complete = false;
        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, /*sign=*/false, /*bip32derivs=*/true, /*n_signed=*/nullptr, psbtx, complete);
        assert(!complete);
        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.

  31. luke-jr commented at 10:15 PM on March 14, 2022: member

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

  32. Sjors force-pushed on Mar 15, 2022
  33. 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.

  34. Sjors force-pushed on Mar 15, 2022
  35. Sjors force-pushed on Mar 15, 2022
  36. 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)

  37. luke-jr changes_requested
  38. refactor: helper function signWithExternalSigner()
    Does not change behavior.

Review hint:
git show --color-moved --color-moved-ws=allow-indentation-change
    4b5a6cd149
  39. 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
  40. Sjors force-pushed on Mar 16, 2022
  41. luke-jr approved
  42. luke-jr commented at 4:54 PM on March 16, 2022: member

    utACK 2efdfb88aab6496dcf2b98e0de30635bc6bade85

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

  44. hebasto added the label Needs backport (23.x) on Mar 17, 2022
  45. hebasto merged this on Mar 17, 2022
  46. hebasto closed this on Mar 17, 2022
  47. hebasto referenced this in commit fc421d4c8c on Mar 17, 2022
  48. hebasto referenced this in commit 940694664d on Mar 17, 2022
  49. hebasto referenced this in commit 642f2726de on Mar 17, 2022
  50. hebasto commented at 6:35 AM on March 17, 2022: member

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

  51. hebasto removed the label Needs backport (23.x) on Mar 17, 2022
  52. jonatack commented at 8:49 AM on March 17, 2022: contributor

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

  53. Sjors commented at 9:31 AM on March 17, 2022: member

    @achow101?

  54. sidhujag referenced this in commit e19f98b3d3 on Mar 18, 2022
  55. MarcoFalke referenced this in commit 7d03cf632d on Mar 24, 2022
  56. fujicoin referenced this in commit 85b3b04392 on Mar 25, 2022
  57. fujicoin referenced this in commit 7866b8850a on Mar 25, 2022
  58. fujicoin referenced this in commit 47c94164ad on Mar 25, 2022
  59. backpacker69 referenced this in commit a3ffd89267 on Jan 18, 2023
  60. backpacker69 referenced this in commit 078cd1ce30 on Jan 18, 2023
  61. backpacker69 referenced this in commit a77578477f on Jan 18, 2023
  62. bitcoin-core locked this on Jun 5, 2023
Contributors
SjorshebastopromagDrahtBotjonatackkristapskluke-jr


achow101

Labels
WalletUX

Milestone
23.0

Linked (view graph)
#441 Add Create Unsigned button to SendConfirmationDialog#551 External signer Send button disabled
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 15:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me