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

pull krab wants to merge 1 commits into bitcoin:master from krab:qt5-uri-error-message changing 3 files +7 −10

krab commented at 11:16 AM on March 19, 2018: contributor

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 added the label GUI on Mar 19, 2018
krab commented at 2:24 PM on March 19, 2018: contributor

Travis CI failed on 30 minutes timeout in multiple jobs.
laanwj commented at 2:53 PM on March 19, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12723/commits/98dd40ee5dc5faf83c88672437f639272ca76a4f - needs testing, but this seems to implement #11645.

in src/qt/guiutil.cpp:217 in 7a1eaed4c0 outdated

 211 | @@ -212,11 +212,14 @@ bool parseBitcoinURI(QString uri, SendCoinsRecipient *out)
 212 |      // Convert bitcoin:// to bitcoin:
 213 |      //
 214 |      //    Cannot handle this later, because bitcoin:// will cause Qt to see the part after // as host,
 215 | -    //    which will lower-case it (and thus invalidate the address).
 216 | +    //    which will lower-case it (and thus invalidate the address). Workaround only for Qt4 support.
 217 | +
 218 | +#if QT_VERSION < 0x050000

laanwj commented at 2:55 PM on March 19, 2018:

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 commented at 5:47 PM on March 19, 2018: member

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 commented at 5:56 AM on March 20, 2018: member

Concept ACK The URI tests need updating:

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

in src/qt/paymentserver.cpp:409 in 98dd40ee5d outdated

 403 | @@ -404,7 +404,12 @@ void PaymentServer::handleURIOrFile(const QString& s)
 404 |          return;
 405 |      }
 406 |  
 407 | -    if (s.startsWith(BITCOIN_IPC_PREFIX, Qt::CaseInsensitive)) // bitcoin: URI
 408 | +    if (s.startsWith("bitcoin://", Qt::CaseInsensitive))
 409 | +    {
 410 | +        Q_EMIT message(tr("URI handling"), tr("'bitcoin://' it's not a correct URI. Use 'bitcoin:' instead."),

fanquake commented at 5:56 AM on March 20, 2018:

nit s/its/is

jonasschnelli commented at 11:50 AM on March 20, 2018: contributor

utACK 2ad827fd6a3a9d59a13b7226d9159929ec5ab718
laanwj commented at 1:10 PM on March 20, 2018: member

@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 commented at 1:18 PM on March 20, 2018: member

utACK after squash, this should be one commit
krab commented at 1:46 PM on March 20, 2018: contributor

i'm not sure if it's possible to squash all commits into single one without recreating PR
laanwj commented at 2:28 PM on March 20, 2018: member

That is possible, we do it all the time. See here: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
krab force-pushed on Mar 20, 2018
fanquake commented at 7:11 AM on March 21, 2018: 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(9b8b1079ddab64ac955766536c38d23dc57bc499) "bitcoin:" : <img width="831" alt="" src="https://user-images.githubusercontent.com/863730/37697620-c30f3136-2d18-11e8-8eed-48178b0e6693.png">

master(9b8b1079ddab64ac955766536c38d23dc57bc499) "bitcoin://" : <img width="830" alt="" src="https://user-images.githubusercontent.com/863730/37697628-cc9400ec-2d18-11e8-8e4c-8cc4e3e56243.png">

This PR: 7ddb674 "bitcoin:" : <img width="828" alt="" src="https://user-images.githubusercontent.com/863730/37697729-7209ed3e-2d19-11e8-8aca-fce8c0030559.png">

7ddb674 "bitcoin://" : <img width="829" alt="" src="https://user-images.githubusercontent.com/863730/37697733-796ece14-2d19-11e8-9aea-69a8c955a5bd.png">

My only nit would be to swap 'correct' for 'valid' in "'bitcoin://' is not a correct URI". tACK 7ddb674b55b99ee645517699a4946cb841417cee
Qt: Warn users about invalid-BIP21 URI bitcoin:// b7fbcc53d0
krab force-pushed on Mar 21, 2018
Sjors commented at 2:01 PM on March 21, 2018: member

tACK b7fbcc5
fanquake deleted a comment on Mar 21, 2018
laanwj merged this on Mar 21, 2018
laanwj closed this on Mar 21, 2018
laanwj referenced this in commit 310dc61ea3 on Mar 21, 2018
luke-jr commented at 4:41 PM on June 12, 2018: member

This seems to cause the URI to fail, not just warn. It also affects Qt4 and Qt5 equally. Am I missing something?
Sjors commented at 9:42 AM on June 13, 2018: member

@luke-jr it already failed, but with an incorrect error message. Now it fails with a correct error message.
luke-jr commented at 3:44 PM on June 13, 2018: member

I thought this logic avoided the error?
Sjors commented at 12:03 PM on June 14, 2018: member

@luke-jr no, the error happens before that code is reached. See #11645
PastaPastaPasta referenced this in commit 898423ccad on Dec 16, 2020
PastaPastaPasta referenced this in commit 8f6228bb2c on Dec 18, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11645 BitcoinQT downcases bitcoin://... address on OSX