Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve user dialog when signing multisig psbts #832

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mjdietzx
Copy link
Contributor

Prior to this commit, when a psbt requiring multiple signers is signed via gui the dialogue is vague/confusing. Whether the signer successfully partially signs the psbt or not the same warning is displayed "Could not sign any more inputs." After signing the tx, the only way for the user to know if their action had any effect is to inspect the psbt (unless it was the last signature that completed the psbt, in which case there is a success dialoge).

Now we indicate when a psbt has been partially signed, and show how many partial signatures the psbt has in the transaction description.

Here is what a "daisy chain" signing flow for a 4-of-4 multisig looks like via gui. Same as in bitcoin core's functional test wallet_multisig_descriptor_psbt

First signer:
image

Second signer:
image

Third signer:
image

Fourth and final signer (psbt is complete and can be broadcasted):
image

Before this commit, the first-third signers would just see this after signing. It is at best confusing because it is the same warning a signer would see if they were not in the multisig and no signature was added to the psbt:
image

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 30, 2024

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

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK pablomartin4btc, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin-core/gui/runs/28127968944

Hints

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Prior to this commit, when a psbt requiring multiple signers
is signed via gui the dialog is vague/confusing. Whether the
signer successfully partially signs the psbt or not the same warning
is displayed "Could not sign any more inputs." After signing the tx,
the only way for the user to know if their action had any effect is
to inspect the psbt (unless it was the last signature that completed
the psbt, in which case there is a success dialog).

Now we indicate when a psbt has been partially signed, and show how many
partial signatures the psbt has in the transaction description.
@mjdietzx mjdietzx force-pushed the sign_multisig_psbts_better_dialog branch from afca742 to 8aec372 Compare July 31, 2024 01:13
@hebasto hebasto changed the title gui: improve user dialog when signing multisig psbts Improve user dialog when signing multisig psbts Jul 31, 2024
@hebasto
Copy link
Member

hebasto commented Jul 31, 2024

cc @achow101 @furszy

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/qt/psbtoperationsdialog.cpp Show resolved Hide resolved
@pablomartin4btc
Copy link
Contributor

It would be interesting to see a PSBT window's output sample when the user signed an input but couldn't sign another input which is also part of the transaction? (Is that possible?)

@mjdietzx
Copy link
Contributor Author

mjdietzx commented Jul 31, 2024

It would be interesting to see a PSBT window's output sample when the user signed an input but couldn't sign another input which is also part of the transaction? (Is that possible?)

It's possible but I considered it outside the scope of this PR - in such cases behavior is not changed by this PR. Because i want to keep this simple (seems trying to handle cases like this and communicate it to the user in a straightforward manner would be complicated).

In this PR I just try to improve the UX for "normal" multisigs like wallet_multisig_descriptor_psbt.py and bitcoin/bitcoin#29156 - I don't think it can ever be the case that one input is signed but another is not, and I'd expect this to be true for vast majority of wallets where users are using bitcoin core's gui to sign psbts

All that said, interested to see what reviewers with more experience than me think

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants