search

bitcoin-core / gui

Bitcoin Core GUI staging repository
https://github.com/bitcoin/bitcoin
MIT License
604 stars 266 forks source link

Improve user dialog when signing multisig psbts #832

Open mjdietzx opened 3 months ago

mjdietzx commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

🚧 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.
hebasto commented 3 months ago

cc @achow101 @furszy

pablomartin4btc commented 3 months ago

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 commented 3 months ago

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 https://github.com/bitcoin/bitcoin/pull/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