Fix misleading signmessage error with segwit

willcl-ark commented at 1:02 pm on April 29, 2024: member

As described in https://github.com/bitcoin/bitcoin/issues/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 https://github.com/bitcoin/bitcoin/issues/10542 ?

DrahtBot commented at 1:02 pm on April 29, 2024: contributor

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.

in src/qt/signverifymessagedialog.cpp:128 in 93c8252912 outdated

122@@ -123,7 +123,10 @@ void SignVerifyMessageDialog::on_signMessageButton_SM_clicked()
123     if (!pkhash) {
124         ui->addressIn_SM->setValid(false);
125         ui->statusLabel_SM->setStyleSheet("QLabel { color: red; }");
126-        ui->statusLabel_SM->setText(tr("The entered address does not refer to a key.") + QString(" ") + tr("Please check the address and try again."));
127+        ui->statusLabel_SM->setText(
128+            tr("The entered address does not refer to a legacy (P2PKH) key.") + QString(" ") +
129+            tr("Message signing for segwit addresses (addresses starting with bc1/tb1) is not supported in this version of Bitcoin Core.") + QString(" ") +

MarnixCroes commented at 5:39 pm on April 29, 2024:

it doesn’t work for P2SH either, which doesn’t start with bc1/tb1
I’d probably leave out the testnet/signet example/mentioning of tb1, to prevent confusion, or remove addresses staring with... in full
nit: maybe write segwit with capitals S and W as used in the rest of the UI

willcl-ark commented at 8:20 am on April 30, 2024:

Taken all suggestions, thanks.

MarnixCroes commented at 6:49 pm on April 29, 2024: contributor

cack

willcl-ark force-pushed on Apr 30, 2024

MarnixCroes approved

MarnixCroes commented at 4:41 pm on April 30, 2024: contributor

lgtm 83def1c5a3741878aa63e6f28cc3def99f76c358 useful clarification

BrandonOdiwuor approved

BrandonOdiwuor commented at 5:29 pm on April 30, 2024: contributor

Code Review ACK 83def1c5a3741878aa63e6f28cc3def99f76c358

hebasto renamed this:
~~gui: fix misleading signmessage error with segwit~~
Fix misleading signmessage error with segwit
on May 2, 2024

hebasto commented at 10:21 am on May 2, 2024: member

Approach ACK 83def1c5a3741878aa63e6f28cc3def99f76c358 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/.

hebasto commented at 10:24 am on May 2, 2024: member

And “Bitcoin Core” phrase should be replaced with PACKAGE_NAME (see https://github.com/bitcoin/bitcoin/pull/18646, https://github.com/bitcoin/bitcoin/pull/19282 etc).

willcl-ark force-pushed on May 2, 2024

willcl-ark commented at 1:56 pm on May 2, 2024: member

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 added the label CI failed on May 2, 2024

DrahtBot commented at 4:13 pm on May 2, 2024: contributor

🚧 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

hebasto commented at 4:25 pm on May 2, 2024: member

Approach ACK 0fea73fc7444884dc6831b1fe9e608898a398a92.

Linter failure?

willcl-ark force-pushed on May 2, 2024

willcl-ark commented at 8:17 pm on May 2, 2024: member

Sorry, I’ve addressed the linter now.

in src/qt/signverifymessagedialog.cpp:13 in 8428fa317e outdated

 9@@ -10,6 +10,7 @@
10 #include <qt/platformstyle.h>
11 #include <qt/walletmodel.h>
12 
13+#include <config/bitcoin-config.h>

hebasto commented at 8:42 pm on May 2, 2024:

It is https://github.com/bitcoin/bitcoin/pull/29494 goal to include this headers unguarded :)

Effectively, this PR conflicts with it. However, I believe it will be merged soon.

Anyway, it seems reasonable in this PR to use:

0#if defined(HAVE_CONFIG_H)
1#include <config/bitcoin-config.h>
2#endif

or

0#include <config/bitcoin-config.h> // IWYU pragma: keep

willcl-ark commented at 7:38 am on May 3, 2024:

Thanks hebasto, I hadn’t seen that in progress. Went for the latter suggestion in fb9f150759b22772dd48983a2be1ea397245e289 as it seems to be preferred by marco in the other PR.

DrahtBot removed the label CI failed on May 2, 2024

gui: fix misleading signmessage error with segwit

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.

fb9f150759

willcl-ark force-pushed on May 3, 2024

hebasto approved

hebasto commented at 8:19 pm on May 7, 2024: member

ACK fb9f150759b22772dd48983a2be1ea397245e289.

DrahtBot requested review from BrandonOdiwuor on May 7, 2024

hebasto merged this on May 7, 2024

hebasto closed this on May 7, 2024

Fix misleading signmessage error with segwit #819

Reviews