Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: When BIP70 is disabled, get PaymentRequest merchant using string search #16852

Merged
merged 1 commit into from Oct 1, 2019

Conversation

@achow101
Copy link
Member

commented Sep 11, 2019

The merchant name is stored in the X.509 certificate embedded in a PaymentRequest. Use some string searching to locate it so that it can be shown to the user in the transaction details when BIP70 support was not configured.

An additional notice is added to the merchant string that indicates the certificate was not verified. When BIP70 is enabled, the certificate would be verified and the merchant name not shown if the certificate was invalid.

src/qt/transactiondesc.cpp Outdated Show resolved Hide resolved
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Concept ACK

Are there other places where the vendor name is used? For ex. the transaction list?

@fanquake fanquake added the GUI label Sep 11, 2019
@achow101 achow101 force-pushed the achow101:bip70-merchant-decode branch from ed4e841 to fc295e4 Sep 11, 2019
@achow101

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Are there other places where the vendor name is used? For ex. the transaction list?

AFAIK, no. @luke-jr might know as he was the one that pointed this out to me.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

AFAIK, no. luke-jr might know as he was the one that pointed this out to me.

I checked: you're right, the only place outside of paymentrequestplus / paymentserver and the tests where the getMerchant method is used is here.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Now, we need to find someone that has actual paid payment requests in their wallet to test this 😄

@laanwj laanwj referenced this pull request Sep 12, 2019
1 of 3 tasks complete
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

I think for testing, a simple option is @gavinandresen php script: https://github.com/gavinandresen/paymentrequest

@achow101

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

For testing, I made an account on https://test.bitpay.com/ and created testnet payment requests there. It says you have to "verify" your account before you can make any payment requests, but for the testing site, you can enter pretty much anything and it will be accepted (except for the email, you have to use a valid email and verify it). Or you can try it out on mainnet using https://bitpay.com/demos. With the mainnet payment requests, you can also modify the request to be for testnet (and thus have an invalid signature) and see how the display is different depending on the valid-ness of the request.

@harding

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Tested fc295e4 works for me on transactions I made via BIP70 back in 2015.

2019-09-14-16_39_16_397011134

@fanquake fanquake added this to the 0.19.0 milestone Sep 15, 2019
@fanquake

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Thanks for doing some "real world" testing @harding.

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Concept ACK.

Tested fc295e4 on macOS 10.14 without BIP70 support by looking at some BitPay transactions from late 2018. It shows the merchant with "(Certificate was not verified)"

However when I test with BIP70 support it doesn't show the merchant. That doesn't work on master either.

A test case for GetPaymentRequestMerchant would be nice.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

However when I test with BIP70 support it doesn't show the merchant. That doesn't work on master either.

Presumably the certificate is no longer valid. I believe they have a new certificate since earlier this year, so the old one expired and thus the merchant name isn't being shown now.

@@ -255,26 +285,34 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall
strHTML += "<b>" + tr("Output index") + ":</b> " + QString::number(rec->getOutputIndex()) + "<br>";

// Message from normal bitcoin:URI (bitcoin:123...?message=example)
for (const std::pair<std::string, std::string>& r : orderForm)
for (const std::pair<std::string, std::string>& r : orderForm) {

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 21, 2019

Member

This will change Message,Message,Merchant,Merchant into Message,Merchant,Message,Merchant.

Is that intentional? Probably should be in a separate commit...

This comment has been minimized.

Copy link
@achow101

achow101 Sep 21, 2019

Author Member

Does it really matter that much that the order is changed?

The only reason I changed it is because it looked funny that there were two loops in a row over the same vector.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019
The merchant name is stored in the X.509 certificate embedded in a
PaymentRequest. Use some string searching to locate it so that it
can be shown to the user in the transaction details when BIP70 support
was not configured.

Github-Pull: bitcoin#16852
Rebased-From: fc295e4
@bitcoin bitcoin deleted a comment from wiranphowan Sep 21, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

certificate is no longer valid. I believe they have a new certificate since earlier this year, so the old one expired and thus the merchant name isn't being shown now.

Hehe so this actually tends to work more reliably because it doesn't check the signature. Which is fine as it was (presumably) checked at the time the payment was made, which is what counts.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

@achow101 Can you do the small comment correction please ^^

If not, this will be merged nevertheless in the course of today or tomorrow before the 0.19 split-off. It's more desirable to have this with a comment typo than not at all.

The merchant name is stored in the X.509 certificate embedded in a
PaymentRequest. Use some string searching to locate it so that it
can be shown to the user in the transaction details when BIP70 support
was not configured.
@achow101 achow101 force-pushed the achow101:bip70-merchant-decode branch from fc295e4 to 85973bc Sep 30, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

ACK 85973bc

laanwj added a commit that referenced this pull request Oct 1, 2019
…t using string search

85973bc When BIP70 is disabled, get PaymentRequest merchant using string search (Andrew Chow)

Pull request description:

  The merchant name is stored in the X.509 certificate embedded in a PaymentRequest. Use some string searching to locate it so that it can be shown to the user in the transaction details when BIP70 support was not configured.

  An additional notice is added to the merchant string that indicates the certificate was not verified. When BIP70 is enabled, the certificate would be verified and the merchant name not shown if the certificate was invalid.

ACKs for top commit:
  laanwj:
    ACK 85973bc

Tree-SHA512: 50fdb60d418e2f9eb65a4b52477be16189f00bfc30493adb27d9fb62100fd5bca33b98b8db6caa8485db424838d3b7a1da802c14ff4917943464401f47391616
@laanwj laanwj merged commit 85973bc into bitcoin:master Oct 1, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.