Restore Send button when using external signer #555

Sjors · 2022-02-21T12:54:59Z

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 · 2022-02-21T13:27:44Z

Concept ACK.

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

promag

Concept ACK

src/qt/sendcoinsdialog.cpp

promag · 2022-02-22T22:01:33Z

src/qt/sendcoinsdialog.cpp

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


a49bb0c

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) { // ... }

This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change

Sjors · 2022-02-24T11:42:34Z

Addressed @promag's feedback.

DrahtBot · 2022-03-02T05:49:20Z

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

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

promag · 2022-03-06T23:09:28Z

src/qt/sendcoinsdialog.cpp

@@ -502,19 +502,34 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
    }

    bool send_failure = false;
-    if (retval == QMessageBox::Save) {
+    bool broadcast = true;


33d165d

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; } }

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.

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

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

Thanks, LGTM, will re-test.

jonatack · 2022-03-10T19:08:24Z

Concept ACK

kristapsk · 2022-03-10T21:27:40Z

Concept ACK

promag

Tested ACK 86e3e2c.

jonatack

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

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

src/qt/sendcoinsdialog.h

src/qt/sendcoinsdialog.cpp

src/qt/sendcoinsdialog.h

src/qt/sendcoinsdialog.cpp

src/qt/sendcoinsdialog.h

luke-jr

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

luke-jr · 2022-03-14T21:58:48Z

src/qt/sendcoinsdialog.cpp

-        bool complete = false;
-        // Always fill without signing first. This prevents an external signer
-        // from being called prematurely and is not expensive.
-        TransactionError err = model->wallet().fillPSBT(SIGHASH_ALL, false /* sign */, true /* bip32derivs */, nullptr, psbtx, complete);


e1c1c61 claims "Does not change behavior.", yet this is no longer done for wallets without an external signer.

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

It leads to some duplication in 0538614, 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.

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)

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

Sjors · 2022-03-15T15:42:13Z

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

luke-jr · 2022-03-15T23:17:18Z

src/qt/sendcoinsdialog.cpp

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


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

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

Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change

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 bitcoin-core#551 Co-authored-by: Jon Atack <jon@atack.com>

luke-jr

utACK 2efdfb8

jonatack · 2022-03-16T19:53:31Z

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

This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Github-Pull: bitcoin-core/gui#555 Rebased-From: 026b5b4

Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Github-Pull: bitcoin-core/gui#555 Rebased-From: 4b5a6cd

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 bitcoin#551 Co-authored-by: Jon Atack <jon@atack.com> Github-Pull: bitcoin-core/gui#555 Rebased-From: 2efdfb8

hebasto · 2022-03-17T06:35:49Z

Backported in bitcoin/bitcoin#24596.

jonatack · 2022-03-17T08:49:45Z

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

Sjors · 2022-03-17T09:31:27Z

@achow101?

70f2c57 options: flip listenonion to false if not listening (Vasil Dimov) 642f272 gui: restore Send for external signer (Sjors Provoost) 9406946 refactor: helper function signWithExternalSigner() (Sjors Provoost) fc421d4 move-only: helper function to present PSBT (Sjors Provoost) Pull request description: Backports from the GUI repo: - bitcoin-core/gui#555 - bitcoin-core/gui#568 ACKs for top commit: Sjors: utACK 70f2c57 gruve-p: ACK 70f2c57 Tree-SHA512: 883c442f8b789a9d11c949179e4382843cbb979a89a625bef3f481c7070421681d9db2af0e5b2449abca362c8ba05cf61db5893aeb6a9237b02088c2fb71e93e

This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Conflicts: src/qt/sendcoinsdialog.cpp Github-Pull: bitcoin-core/gui#555 Rebased-From: 026b5b4523317fdefc69cf5cec55f76f18ad0c0a

Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Github-Pull: bitcoin-core/gui#555 Rebased-From: 4b5a6cd14967b8ec3cb525e4cb18628de6c15091

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> Github-Pull: bitcoin-core/gui#555 Rebased-From: 2efdfb88aab6496dcf2b98e0de30635bc6bade85

This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Github-Pull: bitcoin-core/gui#555 Rebased-From: 026b5b4523317fdefc69cf5cec55f76f18ad0c0a

Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change Github-Pull: bitcoin-core/gui#555 Rebased-From: 4b5a6cd14967b8ec3cb525e4cb18628de6c15091

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> Github-Pull: bitcoin-core/gui#555 Rebased-From: 2efdfb88aab6496dcf2b98e0de30635bc6bade85

hebasto added this to the 23.0 milestone Feb 21, 2022

hebasto added UX All about "how to get things done" Wallet labels Feb 21, 2022

Sjors requested a review from achow101 February 21, 2022 14:52

promag reviewed Feb 22, 2022

View reviewed changes

move-only: helper function to present PSBT

026b5b4

This commit does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change

Sjors force-pushed the 2022/02/send_button branch from a49bb0c to 33d165d Compare February 24, 2022 11:42

DrahtBot mentioned this pull request Mar 2, 2022

Warn when sending to already-used Bitcoin addresses #562

Closed

promag reviewed Mar 6, 2022

View reviewed changes

Sjors changed the title ~~Enable Send button when using external signer~~ Restore Send button when using external signer Mar 10, 2022

Sjors force-pushed the 2022/02/send_button branch 2 times, most recently from 4ed2a3b to 86e3e2c Compare March 10, 2022 21:21

promag approved these changes Mar 11, 2022

View reviewed changes

jonatack reviewed Mar 11, 2022

View reviewed changes

luke-jr reviewed Mar 14, 2022

View reviewed changes

Sjors force-pushed the 2022/02/send_button branch from 86e3e2c to 225794f Compare March 15, 2022 15:42

Sjors force-pushed the 2022/02/send_button branch 2 times, most recently from 347871f to 0538614 Compare March 15, 2022 15:49

luke-jr suggested changes Mar 15, 2022

View reviewed changes

Sjors and others added 2 commits March 16, 2022 10:28

refactor: helper function signWithExternalSigner()

4b5a6cd

Does not change behavior. Review hint: git show --color-moved --color-moved-ws=allow-indentation-change

Sjors force-pushed the 2022/02/send_button branch from 0538614 to 2efdfb8 Compare March 16, 2022 09:29

luke-jr approved these changes Mar 16, 2022

View reviewed changes

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

hebasto merged commit aece566 into bitcoin-core:master Mar 17, 2022

hebasto mentioned this pull request Mar 17, 2022

[23.x] GUI backports bitcoin/bitcoin#24596

Merged

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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 18, 2022

Merge bitcoin-core/gui#555: Restore Send button when using external s…

e19f98b

Sjors mentioned this pull request Apr 25, 2022

rpc, gui: bumpfee signer support bitcoin/bitcoin#21576

Merged

Sjors mentioned this pull request May 10, 2022

Add external signer taproot support bitcoin/bitcoin#23578

Merged

bitcoin-core locked and limited conversation to collaborators Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore Send button when using external signer #555

Restore Send button when using external signer #555

Sjors commented Feb 21, 2022

hebasto commented Feb 21, 2022

promag left a comment

promag Feb 22, 2022

Sjors commented Feb 24, 2022

DrahtBot commented Mar 2, 2022

promag left a comment

promag Mar 6, 2022

Sjors Mar 10, 2022

promag Mar 10, 2022

Sjors Mar 10, 2022

promag Mar 10, 2022

jonatack commented Mar 10, 2022

kristapsk commented Mar 10, 2022

promag left a comment

jonatack left a comment •

edited

Loading

luke-jr left a comment

luke-jr Mar 14, 2022

Sjors Mar 15, 2022 •

edited

Loading

jonatack Mar 16, 2022 •

edited

Loading

Sjors Mar 16, 2022

Sjors commented Mar 15, 2022

luke-jr Mar 15, 2022

Sjors Mar 16, 2022 •

edited

Loading

luke-jr left a comment

jonatack commented Mar 16, 2022

hebasto commented Mar 17, 2022

jonatack commented Mar 17, 2022

Sjors commented Mar 17, 2022

Restore Send button when using external signer #555

Restore Send button when using external signer #555

Conversation

Sjors commented Feb 21, 2022

hebasto commented Feb 21, 2022

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Feb 24, 2022

DrahtBot commented Mar 2, 2022

Conflicts

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatack commented Mar 10, 2022

kristapsk commented Mar 10, 2022

promag left a comment

Choose a reason for hiding this comment

jonatack left a comment • edited Loading

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

jonatack Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Mar 15, 2022

Choose a reason for hiding this comment

Sjors Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

jonatack commented Mar 16, 2022

hebasto commented Mar 17, 2022

jonatack commented Mar 17, 2022

Sjors commented Mar 17, 2022

jonatack left a comment •

edited

Loading

Sjors Mar 15, 2022 •

edited

Loading

jonatack Mar 16, 2022 •

edited

Loading

Sjors Mar 16, 2022 •

edited

Loading