Skip to content

Conversation

willcl-ark
Copy link
Member

As described in bitcoin/bitcoin#10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.

Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.

This change takes the suggested wording from @adiabat.

Perhaps with this we can close bitcoin/bitcoin#10542 ?

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 29, 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
ACK hebasto
Stale ACK BrandonOdiwuor

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

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

cack

@willcl-ark willcl-ark force-pushed the signmessage-error-fix branch from 93c8252 to 83def1c Compare April 30, 2024 08:20
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

lgtm 83def1c
useful clarification

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.

Code Review ACK 83def1c

@hebasto hebasto changed the title gui: fix misleading signmessage error with segwit Fix misleading signmessage error with segwit May 2, 2024
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 83def1c on the content of the messages.

However, splitting lines makes translation work harder due to providing less context. Could you make every touched message a single translatable string?

Also, if you'd like, add translation comment will be very useful to avoid any potential issue similar to (for example, https://github.com/bitcoin/bitcoin/commit/.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

And "Bitcoin Core" phrase should be replaced with PACKAGE_NAME (see bitcoin/bitcoin#18646, bitcoin/bitcoin#19282 etc).

@willcl-ark willcl-ark force-pushed the signmessage-error-fix branch from 83def1c to 0fea73f Compare May 2, 2024 13:56
@willcl-ark
Copy link
Member Author

Hi @hebasto I took your suggestions save for the extra context comments -- I think the text as one blob like this is pretty self-explanatory?

@DrahtBot
Copy link
Contributor

DrahtBot commented May 2, 2024

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

Debug: https://github.com/bitcoin-core/gui/runs/24512951817

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 0fea73f.

Linter failure?

@willcl-ark willcl-ark force-pushed the signmessage-error-fix branch from 0fea73f to 8428fa3 Compare May 2, 2024 20:09
@willcl-ark
Copy link
Member Author

Sorry, I've addressed the linter now.

@DrahtBot DrahtBot removed the CI failed label May 2, 2024
As described in #10542 (and numerous other places), message signing in
Bitcoin Core only supports message signing using P2PKH addresses, at
least until a new message-signing standard is agreed upon.

Therefore update the possibly-misleading error message presented to the
user in the GUI to detail more specifically the reason their message
cannot be signed, in the case that a non P2PKH address is entered.
@willcl-ark willcl-ark force-pushed the signmessage-error-fix branch from 8428fa3 to fb9f150 Compare May 3, 2024 07:37
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fb9f150.

@DrahtBot DrahtBot requested a review from BrandonOdiwuor May 7, 2024 20:19
@hebasto hebasto merged commit 4e56df8 into bitcoin-core:master May 7, 2024
@bitcoin-core bitcoin-core locked and limited conversation to collaborators May 7, 2025
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.

Signmessage doesn't work with segwit addresses
5 participants