qt: Remove network detection based on address in BIP21 #563

1. 
   laanwj commented at 6:50 pm on March 8, 2022: member

   This is removes some ugly and brittle code that switches the global network to testnet based on a provided address. I think in practice it’s very unlikely for testnet BIP21 payment URIs to be used, and if so it’s for testing so it’s easy enough to manually copy it. Or to specify -testnet explicitly.

   There is already no such case for -regtest or -signet.

   After this change it will only accept addresses for the explicitly selected network. Others will result in a “wrong network” popup.

   There is also a possibility for refactor after this as the initialization order of PaymentServer::ipcParseCommandLine isn’t important anymore (well, it still has to be before PaymentServer::ipcSendCommandLine, maybe even merged with it), but I have not done so here.

2. 
   laanwj commented at 6:52 pm on March 8, 2022: member

   Discussion on IRC that triggered this change:

   02022-03-08 18:07:45     dongcarl        Really sus-ed out that paymentserver can call SelectParams and cha
   1nge the global params... That's due to be removed right?
   22022-03-08 19:32:46     laanwj  oh you mean selecting which set of parameters to use based on the provided bitcoin URI 
   32022-03-08 19:33:56     laanwj  to be honest i'm not sure how important that is, who is using testnet bitcoin URIs in a browser or such
   42022-03-08 19:34:09     laanwj  i'd personally be fine with removing that
   52022-03-08 19:36:58     laanwj  i guess you could still explicitly pass -testnet anyway, instead of making it guess from the address?
   62022-03-08 19:37:52     laanwj  the comment is also outdated
   

3. laanwj force-pushed on Mar 8, 2022
4. laanwj added the label Wallet on Mar 8, 2022
5. 
   fanquake commented at 10:05 am on March 9, 2022: member

   cc @dongcarl
6. 
   jonatack commented at 10:24 am on March 9, 2022: contributor

   Code review ACK 2d41f4d90c799d7ed7d853be2417b5093b8dac8a

   There is also a possibility for refactor after this as the initialization order of PaymentServer::ipcParseCommandLine isn’t important anymore

   Was curious about this and 2102ab9f5cb542e6727e0f25e670d8549aa1bf1a looks like the related commit.

   The comment just above in this function looks like it should be moved as well.

   0// Warning: ipcSendCommandLine() is called early in init,
   1// so don't use "Q_EMIT message()", but "QMessageBox::"!
   

7. 
   laanwj commented at 11:11 am on March 9, 2022: member

   The comment just above in this function looks like it should be moved as well.

   I’m not sure. Does the comment refer to the wrong function, or should it be moved? I think it’s valid for both ipcParseCommandLine and ipcSendCommandLine. That said, neither of them actually ever needs to display a warning/error.

   I could update the comment but also remove it.

8. 
   jonatack commented at 11:22 am on March 9, 2022: contributor

   I could update the comment but also remove it.

   Oh sorry, I was thinking more when refactoring as you mentioned; it’s not related to this change.

9. 
   jarolrod commented at 9:30 am on March 10, 2022: member

   crACK https://github.com/bitcoin-core/gui/commit/2d41f4d90c799d7ed7d853be2417b5093b8dac8a
10. kristapsk approved
11. 
    kristapsk commented at 10:58 am on March 10, 2022: contributor

    ACK 2d41f4d90c799d7ed7d853be2417b5093b8dac8a
12. 
    laanwj commented at 11:55 am on March 10, 2022: member

    I hadn’t noticed before that savedPaymentRequests is a static QSet<QString>. Will remove the explicit duplicate detection, which is no longer necessary without the other logic, and push again (sorry).
13. qt: Remove network detection based on address in BIP21

    This is some very ugly and brittle code that switches the global network
    based on a provided address, remove it. I think in practice it's very
    unlikely for testnet BIP21 payment URIs to be used, and if so it's for
    testing so it's easy enough to manually copy it. Or to specify
    `-testnet` explicitly.
    
    There is already no case for `-regtest` or `-signet`.

    b7dbc83f23
14. laanwj force-pushed on Mar 10, 2022
15. 
    jonatack commented at 12:30 pm on March 10, 2022: contributor

    Good point. Verified at https://doc.qt.io/qt-5/qset.html#insert that a QSet behaves (as expected) like a set: “Inserts item value into the set, if value isn’t already in the set.”

    ACK b7dbc83f23f67048cd6f66f5587381d73fad4894

16. 
    achow101 commented at 12:44 pm on March 10, 2022: member

    ACK b7dbc83f23f67048cd6f66f5587381d73fad4894
17. hebasto merged this on Mar 10, 2022
18. hebasto closed this on Mar 10, 2022

    ---

19. sidhujag referenced this in commit 8100ac5b1a on Mar 11, 2022
20. bitcoin-core locked this on Mar 10, 2023

Contributors
laanwj fanquake jonatack jarolrod kristapsk achow101

Labels
Wallet