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

achow101 · 2019-09-11T07:41:52Z

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

laanwj · 2019-09-11T07:43:36Z

Concept ACK

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

achow101 · 2019-09-11T07:58:24Z

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 · 2019-09-11T11:01:26Z

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 · 2019-09-11T15:00:30Z

Concept ACK

laanwj · 2019-09-12T06:56:38Z

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

jonasschnelli · 2019-09-12T12:44:22Z

Concept ACK

jonasschnelli · 2019-09-12T12:45:50Z

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

achow101 · 2019-09-12T15:43:56Z

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 · 2019-09-15T02:43:37Z

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

fanquake · 2019-09-15T10:20:29Z

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

Sjors · 2019-09-16T10:11:29Z

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 · 2019-09-16T15:57:14Z

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.

luke-jr · 2019-09-21T03:20:31Z

src/qt/transactiondesc.cpp

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

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

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.

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: fc295e42071a82955c35d90792a9cbd127ca2859

laanwj · 2019-09-26T12:26:39Z

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.

src/qt/transactiondesc.cpp

laanwj · 2019-09-30T11:47:19Z

@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.

laanwj · 2019-10-01T09:52:58Z

ACK 85973bc

…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

dooglus · 2019-11-14T19:27:43Z

I have a couple of payments to bitpay in my wallet. In Bitcoin Core 0.18.1 the merchant information wasn't displayed at all for either of them. When I viewed the transaction details I would see this in the debug.log:

PaymentRequestPlus::getMerchant: Payment request: certificate expired or not yet active

The certificate was valid from 2017-03-23 18:22:00.000 UTC to 2019-04-25 19:11:00.000 UTC, and so could no longer be validated, and as a result Core wasn't showing any "Merchant:" information at all.

In Bitcoin Core 0.19 I see:

Merchant: bitpay.com (Certificate was not verified)

which is infinitely better.

laanwj · 2019-11-15T11:38:05Z

@dooglus Thanks for checking. Yes it was a bug that the certificate-checking for transactions used the current date, instead of the date of the transaction (also, verifying it once at payment would have been good enough, no need to do so every time details are viewed).

…hant using string search Summary: 85973bcc44f60fe3bbc952557ebf578dd4c475d2 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. --- Backport of Core [[bitcoin/bitcoin#16852 | PR16852]] Test Plan: cmake -GNinja -DENABLE_BIP70=ON|OFF ninja all check Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8751

laanwj reviewed Sep 11, 2019

View reviewed changes

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

fanquake added the GUI label Sep 11, 2019

achow101 force-pushed the bip70-merchant-decode branch from ed4e841 to fc295e4 Compare September 11, 2019 07:57

laanwj mentioned this pull request Sep 12, 2019

build: disable BIP70 support by default #15584

Merged

3 tasks

laanwj mentioned this pull request Sep 12, 2019

[PoC] GUI: Migrate BIP70 merchant info to mapValue["to"] #15064

Closed

fanquake added this to the 0.19.0 milestone Sep 15, 2019

luke-jr reviewed Sep 21, 2019

View reviewed changes

bitcoin deleted a comment from wiranphowan Sep 21, 2019

practicalswift reviewed Sep 28, 2019

View reviewed changes

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

laanwj added the Waiting for author label Sep 30, 2019

achow101 force-pushed the bip70-merchant-decode branch from fc295e4 to 85973bc Compare September 30, 2019 15:32

fanquake removed the Waiting for author label Oct 1, 2019

laanwj merged commit 85973bc into bitcoin:master Oct 1, 2019

laanwj mentioned this pull request Oct 26, 2019

Remove BIP70 support #17165

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

achow101 commented Sep 11, 2019

Uh oh!

Uh oh!

laanwj commented Sep 11, 2019

Uh oh!

achow101 commented Sep 11, 2019

Uh oh!

laanwj commented Sep 11, 2019

Uh oh!

luke-jr commented Sep 11, 2019

Uh oh!

laanwj commented Sep 12, 2019

Uh oh!

jonasschnelli commented Sep 12, 2019

Uh oh!

jonasschnelli commented Sep 12, 2019

Uh oh!

achow101 commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harding commented Sep 15, 2019

Uh oh!

fanquake commented Sep 15, 2019

Uh oh!

Sjors commented Sep 16, 2019

Uh oh!

achow101 commented Sep 16, 2019

Uh oh!

luke-jr Sep 21, 2019

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 21, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 26, 2019

Uh oh!

Uh oh!

laanwj commented Sep 30, 2019

Uh oh!

laanwj commented Oct 1, 2019

Uh oh!

dooglus commented Nov 14, 2019

Uh oh!

laanwj commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

achow101 commented Sep 12, 2019 •

edited

Loading