paymentserver: click-to-pay-handler try both nets #3179

Diapolo commented at 4:48 PM on October 28, 2013: none

it can happen, that PaymentServer::ipcSendCommandLine() is unable to detect the correct network, for example when a bitcoin: URI contains an invalid address or when a payment request can be read
in this case try to send the request to a mainnet local pr server, if this fails use a testnet local pr server

paymentserver: click-to-pay-handler try both nets

- it can happen, that PaymentServer::ipcSendCommandLine() is unable to
  detect the correct network, for example when a bitcoin: URI contains an
  invalid address or when a payment request can be read
- in this case try to send the request to a mainnet local pr server, if this
  fails use a testnet local pr server

928df2a96b

BitcoinPullTester commented at 5:24 PM on October 28, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/928df2a96bdfa27c46fbace43f96f47d652c37e8 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 5:42 PM on October 28, 2013: member

I'm not sure that I understand -- why not simply fail if a bitcoin URI contains an invalid address or the payment request cannot be read?

gavinandresen commented at 1:10 AM on October 30, 2013: contributor

NACK, subtle behavior like this is fertile ground for security issues. If the address or request isn't valid then an error is the right thing to do.

Diapolo commented at 8:32 AM on October 30, 2013: none

@gavinandresen @laanwj See, the problem is, if I supply an invalid address in an URI, but have a running Bitcoin-Qt instance a new one is started, which won't work, because there is one running already ^^. This patch tries to deliver the faulty bitcoin: URI and THEN displays a nice warning message.

laanwj commented at 9:18 AM on October 30, 2013: member

It'd be better to shortcut this. Display the error message immediately without launching or dispatching anything.

Diapolo commented at 10:02 AM on October 30, 2013: none

Good idea, but this is not possible, as GUI hasn't started yet so we can't pop up a message box..

laanwj commented at 10:25 AM on October 30, 2013: member

I know, but there are ways around that; for example you could remember the error, then show it and exit after the GUI is initialized (but before spinning any core stuff).

Diapolo commented at 10:45 AM on October 30, 2013: none

@laanwj Tell me, is it easier/better UX wise to (try to) send an invalid URI to a running server and get a nice error message or show a saved error that the URI could not be parsed (via QMessageBox in init.cpp) and then another error, which tells that you already have a running instance, because when ipcSendCommandLine() fails we try to start a new instance (which also is not recommended to let an instance start by clicking an URI, because of missing command-line switches ^^).

laanwj commented at 11:00 AM on October 30, 2013: member

Eh, I just checked bitcoin.cpp and PaymentServer::ipcSendCommandLine is called after the GUI initialization. The QApplication object is created and translations are loaded, which is far enough along to show a dialog box and exit. What's the problem? There's no need to let it come as far as creating an actual instance. Exit before the second error.

Diapolo commented at 11:14 AM on October 30, 2013: none

I relied on Gavins comment I quoted above GUI hasn't started yet so we can't pop up a message box.. sorry.

laanwj commented at 11:55 AM on October 30, 2013: member

Hehe, yeah, sometimes no comments is better than an outdated, no longer valid comment

Diapolo commented at 1:56 PM on October 30, 2013: none

Can we exchange a few more thoughts on this Please? Take another look at ipcSendCommandLine().

Currently we have 2 checks for normal URIs that can fail GUIUtil::parseBitcoinURI() and CBitcoinAddress::IsValid(). Both of which are used just to detect the active network (for sending the received URI to the TCP local sever), we currently don't use them to reject invalid URIs or show any message.

Now add my change, which just first tries to send to the mainnet TCP local server and if that fails tries testnet TCP local server and if that fails client associated with bitcoin: is started (which, as I said is not recommended).

IMHO it's ugly and unneeded to use static QMessageBox calls to show messages to the user in ipcSendCommandLine(), if we would just say try mainnet first and testnet otherwise and remove the pre-checks (which are done later in the client again, if we parse the URIs for real usage) this would reduce code complexity and allows us to handle all URI or payment-request related checks later when client is fully initialized.

laanwj commented at 2:20 PM on October 30, 2013: member

I understand what you mean but this is a weird edge case anyway: ideally, users should not encounter malformed URIs and payment requests. We shouldn't be spending this much time on it. The only thing that matters here security, and clear feedback that something is wrong. A simple QMessageBox does as well as anything here.

The pre-check here would try to figure out whether the URI/payment request is testnet or mainnet, only if it cannot figure out it should error out prematurely. It does not need to do further checking on the URI, as that can happen later on. It's not really a problem if some input checking happens in two places. Better be safe than sorry.

Diapolo commented at 3:04 PM on October 30, 2013: none

Closed until further notice ^^.

Diapolo closed this on Oct 30, 2013

Diapolo deleted the branch on Oct 30, 2013

DrahtBot locked this on Sep 8, 2021