Skip to content

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

Merged
laanwj merged 1 commit intobitcoin:masterfrom
krab:qt5-uri-error-message
Mar 21, 2018
Merged

Qt5: Warning users about invalid-BIP21 URI bitcoin://#12723
laanwj merged 1 commit intobitcoin:masterfrom
krab:qt5-uri-error-message

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
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

fanquake commented Mar 20, 2018

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
Copy link
Member

@fanquake fanquake Mar 20, 2018

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

jonasschnelli commented Mar 20, 2018

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

fanquake commented Mar 21, 2018

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
@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