Skip to content

Qt5: Warning users about invalid-BIP21 URI bitcoin:// #12723

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 21, 2018
Merged

Qt5: Warning users about invalid-BIP21 URI bitcoin:// #12723

merged 1 commit into from
Mar 21, 2018

Conversation

krab
Copy link
Contributor

@krab krab commented Mar 19, 2018

This change affects only Qt5 users, since Qt4 QUrl don't forces lower case for urls. Also bitcoin-qt builds against Qt4 on linux.

PR for #11645

@fanquake fanquake added the GUI label Mar 19, 2018
@krab
Copy link
Contributor Author

krab commented Mar 19, 2018

Travis CI failed on 30 minutes timeout in multiple jobs.

@laanwj
Copy link
Member

laanwj commented Mar 19, 2018 •
edited
Loading

utACK
98dd40e - needs testing, but this seems to implement #11645.

laanwj
laanwj reviewed Mar 19, 2018
View reviewed changes
src/qt/guiutil.cpp Outdated
// which will lower-case it (and thus invalidate the address).
// which will lower-case it (and thus invalidate the address). Workaround only for Qt4 support.

#if QT_VERSION < 0x050000
Copy link
Member

@laanwj laanwj Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

IMO just drop these lines of code (and comment), so that it gets to the error message for Qt4 too. This makes behavior under both consistent. Also: Qt4 support is under-tested and going to be removed in the not-so-far future, so might be better to not have a special, not strictly necessary workaround for it.

@Sjors
Copy link
Member

Sjors commented Mar 19, 2018

I'll review this later.

@laanwj how likely is it that QT4 will be removed by the next release? I'd rather not test it :-)

@fanquake
Copy link
Member

Concept ACK
The URI tests need updating:

FAIL!  : URITests::uriTests() 'rv.address == QString("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W")' returned FALSE. ()
   Loc: [qt/test/uritests.cpp(55)]

fanquake
fanquake reviewed Mar 20, 2018
View reviewed changes
src/qt/paymentserver.cpp Outdated
if (s.startsWith(BITCOIN_IPC_PREFIX, Qt::CaseInsensitive)) // bitcoin: URI
if (s.startsWith("bitcoin://", Qt::CaseInsensitive))
{
Q_EMIT message(tr("URI handling"), tr("'bitcoin://' it's not a correct URI. Use 'bitcoin:' instead."),
Copy link
Member

Choose a reason for hiding this comment

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

nit s/its/is

@jonasschnelli
Copy link
Contributor

utACK 2ad827fd6a3a9d59a13b7226d9159929ec5ab718

@laanwj
Copy link
Member

laanwj commented Mar 20, 2018

@laanwj how likely is it that QT4 will be removed by the next release? I'd rather not test it :-)

If it was up to me: yesterday.
This discussion is in #8263.

@laanwj
Copy link
Member

laanwj commented Mar 20, 2018

utACK after squash, this should be one commit

@krab
Copy link
Contributor Author

krab commented Mar 20, 2018

i'm not sure if it's possible to squash all commits into single one without recreating PR

@laanwj
Copy link
Member

laanwj commented Mar 20, 2018

That is possible, we do it all the time. See here: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@krab krab force-pushed the qt5-uri-error-message branch from 2ad827f to 7ddb674 Compare March 20, 2018 15:44
@fanquake
Copy link
Member

Tested on macOS 10.13.3 with Qt 5.10.1
All these used

bitcoin:12A1MyfXbW6RhdRAZEqofac5jCQQjwEPBu?amount=1.337&message=Payment&label=Satoshi

with and without '//' as the URI.
Tested master(9b8b107) "bitcoin:" :

master(9b8b107) "bitcoin://" :

This PR:
7ddb674 "bitcoin:" :

7ddb674 "bitcoin://" :

My only nit would be to swap 'correct' for 'valid' in "'bitcoin://' is not a correct URI".
tACK 7ddb674b55b99ee645517699a4946cb841417cee

@krab krab force-pushed the qt5-uri-error-message branch from 7ddb674 to b7fbcc5 Compare March 21, 2018 12:40
@Sjors
Copy link
Member

Sjors commented Mar 21, 2018

tACK b7fbcc5

@bitcoin bitcoin deleted a comment from ilike2bottom40 Mar 21, 2018
@laanwj laanwj merged commit b7fbcc5 into bitcoin:master Mar 21, 2018
laanwj added a commit that referenced this pull request Mar 21, 2018
@laanwj
Merge #12723: Qt5: Warning users about invalid-BIP21 URI bitcoin://
310dc61 
b7fbcc5 Qt: Warn users about invalid-BIP21 URI bitcoin:// (Alexey Ivanov)

Pull request description:

  This change affects only Qt5 users, since Qt4 QUrl don't forces lower case for urls. Also bitcoin-qt builds against Qt4 on linux.

  PR for #11645

Tree-SHA512: 6b8cb18b29dbd2754e190a662ed67274a7f0decc6adb00b7e1af107d5f8ea2845b668cf28d6ccf2f1d15e8ef212f5a76910810634a4c15e7fabd1dd2072e7232
@jnewbery jnewbery mentioned this pull request Mar 27, 2018
Closed
@luke-jr
Copy link
Member

luke-jr commented Jun 12, 2018

This seems to cause the URI to fail, not just warn. It also affects Qt4 and Qt5 equally. Am I missing something?

@Sjors
Copy link
Member

Sjors commented Jun 13, 2018

@luke-jr it already failed, but with an incorrect error message. Now it fails with a correct error message.

@Sjors Sjors mentioned this pull request Jun 13, 2018
Closed
@luke-jr
Copy link
Member

luke-jr commented Jun 13, 2018

I thought this logic avoided the error?

@Sjors
Copy link
Member

Sjors commented Jun 14, 2018

@luke-jr no, the error happens before that code is reached. See #11645

@dantudor dantudor mentioned this pull request Sep 6, 2018
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12723: Qt5: Warning users about invalid-BIP21 URI bitco…
898423c 
…in://

b7fbcc5 Qt: Warn users about invalid-BIP21 URI bitcoin:// (Alexey Ivanov)

Pull request description:

  This change affects only Qt5 users, since Qt4 QUrl don't forces lower case for urls. Also bitcoin-qt builds against Qt4 on linux.

  PR for bitcoin#11645

Tree-SHA512: 6b8cb18b29dbd2754e190a662ed67274a7f0decc6adb00b7e1af107d5f8ea2845b668cf28d6ccf2f1d15e8ef212f5a76910810634a4c15e7fabd1dd2072e7232
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 18, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12723: Qt5: Warning users about invalid-BIP21 URI bitco…
8f6228b 
…in://

b7fbcc5 Qt: Warn users about invalid-BIP21 URI bitcoin:// (Alexey Ivanov)

Pull request description:

  This change affects only Qt5 users, since Qt4 QUrl don't forces lower case for urls. Also bitcoin-qt builds against Qt4 on linux.

  PR for bitcoin#11645

Tree-SHA512: 6b8cb18b29dbd2754e190a662ed67274a7f0decc6adb00b7e1af107d5f8ea2845b668cf28d6ccf2f1d15e8ef212f5a76910810634a4c15e7fabd1dd2072e7232
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@fanquake fanquake fanquake left review comments

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@krab @laanwj @Sjors @fanquake @jonasschnelli @luke-jr