-
Notifications
You must be signed in to change notification settings - Fork 256
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
Fix misleading signmessage error with segwit #819
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cack
93c8252
to
83def1c
Compare
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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/.
There was a problem hiding this 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).
83def1c
to
0fea73f
Compare
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? |
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
There was a problem hiding this 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?
0fea73f
to
8428fa3
Compare
Sorry, I've addressed the linter now. |
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.
8428fa3
to
fb9f150
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fb9f150.
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 ?