[Qt] payment request / server work

Diapolo commented at 9:23 AM on December 12, 2014: none

This continues my work on the payment request / server part of our BIP70 implementation. Please see individual commits for detailed descriptions.

Diapolo commented at 2:23 PM on December 12, 2014: none

Seems I used some Qt5 stuff, need to check ;).

gmaxwell added the label GUI on Jan 9, 2015

Diapolo commented at 1:56 PM on January 10, 2015: none

I'd love to see that reviewed or ACKd @laanwj @gavinandresen, mind taking a look?

in src/qt/paymentrequestplus.h:None in 7f99855185 outdated

  26 | @@ -27,7 +27,6 @@ class PaymentRequestPlus
  27 |      bool SerializeToString(std::string* output) const;
  28 |  
  29 |      bool IsInitialized() const;
  30 | -    QString getPKIType() const;

jonasschnelli commented at 8:44 AM on January 29, 2015:

This method is unused. Is there a plan to make use of it soon? IMO we should keep unused implementation in master at minimum. Better keep this in a separate branch and add it if we use it.

jonasschnelli commented at 8:58 AM on January 29, 2015:

Nevermind. You removed it. Sorry.

jonasschnelli commented at 8:47 AM on January 29, 2015: contributor

Needs rebase.

jonasschnelli commented at 9:07 AM on January 29, 2015: contributor

It's not related to this pull, but better here than in a new issue: I did try creating a payment-request over gavins php tool and selected bitcoincore as merchant. I got a GUI: PaymentRequestPlus::getMerchant : Payment request: empty certificate chain. A) is there no merchant cert behind "bitcoincore"? B) IMO we should show something to the user in case of a invalid or missing cert chain.

Diapolo commented at 7:01 AM on January 30, 2015: none

@jonasschnelli Rebased

As for the cert problems with gavins php tool, IMHO you won't get any authenticated payment request from it and consensus then is to silently treat it as valid unauthenticated request. I had that discussion with @laanwj @gavinandresen and @mikehearn ;).

jonasschnelli commented at 9:11 AM on January 30, 2015: contributor

Tested, reviewed, stepped. Mostly move-only, cleanup things (does that require 7 commits, squash?).

ACK

Diapolo commented at 9:13 AM on January 30, 2015: none

I did each change in a seperate commit to make it easy to review :).

luke-jr commented at 9:21 AM on January 30, 2015: member

Does anyone have a test plan for this?

Diapolo commented at 9:27 AM on January 30, 2015: none

@luke-jr Are you serious??

in src/qt/paymentrequestplus.cpp:None in b1e4965da4 outdated

 118 | @@ -125,7 +119,7 @@ bool PaymentRequestPlus::getMerchant(X509_STORE* certStore, QString& merchant) c
 119 |      // The first cert is the signing cert, the rest are untrusted certs that chain
 120 |      // to a valid root authority. OpenSSL needs them separately.
 121 |      STACK_OF(X509) *chain = sk_X509_new_null();
 122 | -    for (int i = certs.size()-1; i > 0; i--) {
 123 | +    for (size_t i = certs.size() - 1; i > 0; i--) {

laanwj commented at 10:26 AM on January 30, 2015:

NACK on changing this. There is check for certs.empty() that prevents this from becoming a bug, but for loops that count down, I generally prefer to use signed types. In this case, i will never visit 0, but normally a loop like this would look like:

for (int i = certs.size() - 1; i >= 0; i--) {

If you then change the int to e.g. size_t, it will loop forever.

Diapolo commented at 9:49 PM on February 3, 2015:

Reverted!

Diapolo commented at 9:56 PM on February 3, 2015: none

@jonasschnelli If you've got an idea how to group the commits so they can get merged I'll gladly do this. Guess this needs an ACK from @laanwj still :).

Diapolo commented at 10:40 AM on March 18, 2015: none

@laanwj @jonasschnelli Is there some consensus possible to at least pick most of these commits or some, so we can get on here!?

Diapolo commented at 8:40 PM on April 12, 2015: none

I'd like to improve payment request stuff, but not getting on with this is blocking, you know ;).

laanwj commented at 10:56 AM on April 15, 2015: member

ACK apart from the char* change.

[Qt] paymentserver: do not log NULL certificates

- also add a few more comments in PaymentServer::LoadRootCAs

5a53d7cda3

[Qt] paymentserver: better logging of invalid certs

Before and after was tested in Windows:

before:
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate:  ("Microsoft Authenticode(tm) Root Authority")
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate:  ()
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate:  ()
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate:  ()

after:
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate:  "01" ("Microsoft Authenticode(tm) Root Authority")
() ()
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate:  "01" () () ("Copyright (c) 1997 Microsoft Corp.",
"Microsoft Time Stamping Service Root", "Microsoft Corporation")
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate:  "4a:19:d2:38:8c:82:59:1c:a5:5d:73:5f:15:5d:dc:a3" ()
() ("NO LIABILITY ACCEPTED, (c)97 VeriSign, Inc.", "VeriSign Time Stamping
Service Root", "VeriSign, Inc.")
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate:  "e4:9e:fd:f3:3a:e8:0e:cf:a5:11:3e:19:a4:24:02:32" ()
() ("Class 3 Public Primary Certification Authority")

6e17a74766

[Qt] remove unused PaymentRequestPlus::getPKIType function d19ae3cf66

[Qt] take care of a missing typecast in PaymentRequestPlus::getMerchant() 9b14aefee3

[Qt] constify first parameter of processPaymentRequest() 35d15959b0

[Qt] minor comment updates in PaymentServer 06087bda87

[Qt] Use identical strings for expired payment request message

- used in sendcoinsdialog.cpp and paymentserver.cpp
- removes an unneded translation string

6171e494fc

Diapolo commented at 12:33 PM on April 15, 2015: none

@laanwj Thanks for review, I fixed the spelling error and remove the QVariant commit, should be ready now :).

laanwj merged this on Apr 15, 2015

laanwj closed this on Apr 15, 2015

laanwj referenced this in commit bc8535b717 on Apr 15, 2015

Diapolo deleted the branch on Apr 15, 2015

MarcoFalke locked this on Sep 8, 2021

[Qt] payment request / server work - part 2 #5467