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

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

2. fanquake added the label GUI on Mar 19, 2018
3. 
   krab commented at 2:24 pm on March 19, 2018: contributor

   Travis CI failed on 30 minutes timeout in multiple jobs.
4. 
   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.
5. 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.
6. 
   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 :-)
7. 
   fanquake commented at 5:56 am on March 20, 2018: member

   Concept ACK The URI tests need updating:

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

8. 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
9. 
   jonasschnelli commented at 11:50 am on March 20, 2018: contributor

   utACK 2ad827fd6a3a9d59a13b7226d9159929ec5ab718
10. 
    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.

11. 
    laanwj commented at 1:18 pm on March 20, 2018: member

    utACK after squash, this should be one commit
12. 
    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
13. 
    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
14. krab force-pushed on Mar 20, 2018
15. 
    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

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

    with and without ‘//’ as the URI. Tested master(9b8b1079ddab64ac955766536c38d23dc57bc499) “bitcoin:” :

    master(9b8b1079ddab64ac955766536c38d23dc57bc499) “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

16. Qt: Warn users about invalid-BIP21 URI bitcoin:// b7fbcc53d0
17. krab force-pushed on Mar 21, 2018
18. 
    Sjors commented at 2:01 pm on March 21, 2018: member

    tACK b7fbcc5
19. fanquake deleted a comment on Mar 21, 2018
20. laanwj merged this on Mar 21, 2018
21. laanwj closed this on Mar 21, 2018

    ---

22. laanwj referenced this in commit 310dc61ea3 on Mar 21, 2018
23. 
    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?
24. 
    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.
25. 
    luke-jr commented at 3:44 pm on June 13, 2018: member

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

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

Contributors
krab laanwj Sjors fanquake jonasschnelli luke-jr

Labels
GUI