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

Show own outputs on PSBT signing window #740

Merged

Conversation

hernanmarino
Copy link
Contributor

@hernanmarino hernanmarino commented Jun 21, 2023

This fixes #732 .
It allows you to identify your own addresses in the outputs of a transaction in the PSBT signing window. This enables easy identification of change outputs, and prevents certain attacks where someone (co-signers of a multisig, or others ) might trick you into signing a transaction while they are stealing the change, since prior to this modification there was no easy way of knowing this.

The identification of the output is similar to the way this is done in the transaction details window.

A sample output is :

image

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 21, 2023

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
ACK jarolrod, achow101
Concept ACK kallerosenbaum, pablomartin4btc

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

@kallerosenbaum
Copy link

Concept ACK

Tested on regtest. Works as described and it does indeed fix #732.

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.

Tested ACK on signet, both, @kallerosenbaum's use case (where you have a watch-only wallet "online" and you sign the tx "offline", then you will need to broadcast it ofc) and a multisig wallet case mentioned by @hernanmarino in the description of this PR, work as expected.
Perhaps it would be more noticeable if the colour of the added text "(own address)" is changed?
Also, I couldn't find any test using txoutIsMine(), although there are for IsMine().

edit: forgot to mention, for the multisig wallet use case, one should create it with the own private key and the public key/ s from the other signers, so when we select it and check the PBST it will be shown the "own address" text in the tx window.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 4da243b

Tested for functionality, and confirmed new translation added here shows up when building translations.

@jarolrod jarolrod requested a review from achow101 July 5, 2023 07:12
@achow101
Copy link
Member

ACK 4da243b

@DrahtBot DrahtBot removed the request for review from achow101 July 14, 2023 23:00
@hebasto hebasto merged commit 57b8336 into bitcoin-core:master Jul 16, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 17, 2023
4da243b qt: show own outputs on PSBT signing window (Hernan Marino)

Pull request description:

  This fixes bitcoin-core/gui#732 .
  It allows you to identify your own addresses in the outputs of a transaction in the PSBT signing window. This enables easy identification of change outputs, and prevents certain attacks where someone (co-signers of a multisig, or others ) might trick you into signing a transaction while they are stealing the change, since prior to this modification there was no easy way of knowing this.

  The identification of the output is similar to the way this is done in the transaction details window.

  A sample output is :

  ![image](https://github.com/bitcoin-core/gui/assets/87907936/48b8a652-7570-466b-9a34-cc0303c86d8c)

ACKs for top commit:
  achow101:
    ACK 4da243b
  jarolrod:
    ACK 4da243b

Tree-SHA512: fa9901d2acc84472c11afcd0a59a859db598cdf5cea755b492178d3e7434b70d9bd8f554928938a2ff9920c8f397fef814ce14b416556c30fba0c3c1f62cd722
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jul 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign PSBT: Can't verify change output
7 participants