[Qt] ensure payment request network matches client network #3683

Diapolo commented at 5:56 PM on February 16, 2014: none

replaces checks in SendCoinsDialog::handlePaymentRequest() that belong to PaymentServer (normal URIs are special cased, as only an isValid check is done on BTC addresses)
prevents the client to handle payment requests that do not match the clients network and shows an error instead (mainly a problem with drag&drop payment requests onto the client window)
includes some small comment changes also

luke-jr commented at 3:18 AM on February 21, 2014: member

This looks fishy, but I'm not familiar with the code. I wouldn't think the payment protocol conveys a network identifier via addresses. Doesn't it just use scriptPubKeys directly?

Diapolo commented at 8:08 AM on February 21, 2014: none

As written in the commit-msg, try to drag&drop a payment-request for testnet onto a mainnet client window...

Diapolo commented at 8:14 AM on April 1, 2014: none

@laanwj Any feelings on this? @mikehearn Perhaps you?

laanwj commented at 8:23 AM on April 1, 2014: member

Code change seems OK to me, haven't tested

mikehearn commented at 8:24 AM on April 1, 2014: contributor

Don't know enough about the code to comment. @luke-jr BIP70 encodes main/testnet in itself without relying on addresses. That's what's being tested here, the self-declaration.

in src/qt/paymentserver.cpp:None in b9ea9fff53 outdated

 433 | @@ -426,7 +434,10 @@ void PaymentServer::handleURIOrFile(const QString& s)
 434 |          PaymentRequestPlus request;
 435 |          SendCoinsRecipient recipient;
 436 |          if (readPaymentRequest(s, request) && processPaymentRequest(request, recipient))
 437 | -            emit receivedPaymentRequest(recipient);
 438 | +        {

laanwj commented at 8:25 AM on April 1, 2014:

One nit: why don't you move the validatePaymentRequest() stuff to processPaymentRequest()? You always use them together, and processPaymentRequest already does all kinds of validation. IMO it'd sense to add it to there instead of creating a new function.

Diapolo commented at 10:07 AM on April 15, 2014:

Good idea, perhaps it'd make sense to prepend the validation stuff before the current code in processPaymentRequest() also...

laanwj commented at 6:22 AM on May 1, 2014:

Yes I think that'd make sense. Either that, or move all validation to validatePaymentRequest. But the middle ground doesn't make sense :)

laanwj added this to the milestone 0.9.2 on Apr 19, 2014

leofidus commented at 3:51 PM on May 1, 2014: none

As far as I see, this also increases BIP70 conformity: payment requests from unknown networks are now treated as invalid instead of being treated as testnet payment request (BIP70 states "If a client receives a PaymentRequest for a network it does not support it must reject the request.").

Diapolo commented at 1:41 PM on May 2, 2014: none

I can't find the link to @gavinandresen payment-request generator, can someone help :)?

mikehearn commented at 1:44 PM on May 2, 2014: contributor

http://bitcoincore.org/~gavin/createpaymentrequest.php

For a main net payment request I tend to use bitpay:

http://bitgivefoundation.org/donate-now/

Diapolo commented at 2:01 PM on May 2, 2014: none

@mikehearn @gavinandresen There is something wrong with the generator!?

<pre> 2014-05-02 13:57:49 GUI: PaymentServer::handleURIOrFile : fetchRequest( QUrl( "http://bitcoincore.org/~gavin/f.php?h=1c1898d2bef6a1169e9c1e0109ba4c3a" ) ) 2014-05-02 13:58:01 GUI: PaymentRequestPlus::getMerchant : SSL error: unable to get local issuer certificate </pre>

Using https://bitcoincore.org/~gavin/createpaymentrequest.php doesn't change that.

Edit: Also is https://bitcoincore.org using OpenSSL? The server cert wasn't renewed.

Diapolo commented at 2:02 PM on May 5, 2014: none

@laanwj Perhaps you can have a look at the above error or ping Gavin?

laanwj commented at 3:01 PM on May 5, 2014: member

@Diapolo I just tested it, and it worked fine for me. I created a signed payment request for testnet, and put the resulting URI in "Open URI" in master, and it popped up a green payment entry.

Diapolo commented at 6:21 PM on May 5, 2014: none

@laanwj I'm sure it's related to #3684, which adds checks we didn't catch before. Can you try again with both patches applied. Would be rather bad, if our current implementation wouldn't deal with such an error. @mikehearn Ping, what do you think? Is your comment in #3684 still valid in terms of certificate errors and how we should handle them?

laanwj commented at 2:59 PM on May 6, 2014: member

@Diapolo The SSL error that you get is weird, basically it mentions being unable to find a certificate. Which could mean that the needed certificate is missing on your computer. Though that would be strange as it worked before...

The OpenSSL faq mentions this error here: https://www.openssl.org/support/faq.html#USER5

mikehearn commented at 3:02 PM on May 6, 2014: contributor

Yes we should not be showing cert errors to users. Sorry the spec doesn't say this. I will try and get it changed.

Diapolo commented at 7:13 PM on May 6, 2014: none

@mikehearn I'm fine with just logging exact errors, but it seems there is something wrong with a) my code or b) the testnet payment-request generator (or the used cert).

I'm going to push the recent changes to this branch in a few minutes.

[Qt] ensure payment request network matches client network

- replaces checks in SendCoinsDialog::handlePaymentRequest() that belong
  to PaymentServer (normal URIs are special cased, as only an isValid
  check is done on BTC addresses)
- prevents the client to handle payment requests that do not match the
  clients network and shows an error instead (mainly a problem with
  drag&drop payment requests onto the client window)
- includes some small comment changes also

bdc83e8f45

Diapolo commented at 7:23 PM on May 6, 2014: none

@laanwj Not sure about the error I'll try again with just ignoring errors in request.getMerchant() and see what happens.

BitcoinPullTester commented at 7:39 PM on May 6, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/bdc83e8f450456c9f547f1c4eab43571bac631c2 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 3:34 PM on May 9, 2014: member

ACK

laanwj added this to the milestone 0.10.0 on May 13, 2014

laanwj removed this from the milestone 0.9.2 on May 13, 2014

laanwj merged this on Jun 2, 2014

laanwj closed this on Jun 2, 2014

laanwj referenced this in commit 52d7a54434 on Jun 2, 2014

Diapolo deleted the branch on Jun 2, 2014

DrahtBot locked this on Sep 8, 2021