Payment Protocol Work

gavinandresen commented at 6:43 pm on April 18, 2013: contributor

Add support for the Payment Protocol ( https://en.bitcoin.it/wiki/BIP_0070 ) to Bitcoin-Qt.

Web page where you can create test PaymentRequests, signed with either the ‘bitcoincore.org’ website SSL certificate or a ‘gavinandresen@gmail.com’ StartSSL email certificate: https://bitcoincore.org/~gavin/createpaymentrequest.php

Source code for that website is available at: https://github.com/gavinandresen/paymentrequest/tree/master/php/demo_website

Test plan: https://github.com/gavinandresen/QA/blob/master/PaymentRequestTest.md

This adds a dependency to the Bitcoin-Qt build: you need the protocol buffer compiler and library (see doc/readme-qt.rst)

laanwj commented at 5:55 am on April 19, 2013: member

Nice, I’ll do some testing with it over the weekend.

gavinandresen commented at 2:54 pm on April 19, 2013: contributor

Pull-tester failure is because I didn’t update the unit test data when I changed the PaymentRequest protocol buffer format…

gavinandresen commented at 5:35 pm on April 19, 2013: contributor

Unit tests fixed, but I bet the mingw-Windows build will not work because we’ll need a mingw-compiled -lprotobuf

laanwj commented at 2:22 pm on April 21, 2013: member

I’m had some issues building in qt creator (qt creator builds to a different directory than the source directory). I get the following error:

0.../bitcoin/src/qt/paymentrequest.proto: -1: error: File does not reside within any path specified 
1using --proto_path (or -I).  You must specify a --proto_path which encompasses this file.  Note that 
2the proto_path must be an exact prefix of the .proto file names -- protoc is too dumb to figure out when
3 two paths (e.g. absolute and relative) are equivalent (it's harder than you think).

On first look this is weird because –proto_path is provided, and points to the src/qt directory. However, this fails because it is not an exact match. The command line becomes:

0protoc --cpp_out=build --proto_path=src/qt ../bitcoin/src/qt/paymentrequest.proto

So, proto_path is relative to the current working directory (which is the output directory) whereas the proto file is in the source/input directory. I tried to do:

0PROTO_PATH = $$PWD/src/qt

However, this doesn’t solve the problem. It changes the command line to:

0protoc --cpp_out=build --proto_path=/store/orion/projects/bitcoin/bitcoin/src/qt ../bitcoin/src/qt/paymentrequest.proto

So PWD is an absolute path; not the relative path as used for the source files. protoc is still too dumb to understand this.

The only way I could solve this is by changing the line in protobuf.pri to:

0protobuf_decl.commands = $${PROTOC} --cpp_out="$${PROTO_DIR}" $${PROTOPATHS} --proto_path=${QMAKE_FILE_IN_PATH} ${QMAKE_FILE_NAME}

This is pretty much a hack but heh…

mikehearn commented at 12:38 pm on April 24, 2013: contributor

OK, I finally made a payment. Even though I have a fresh wallet that contains only a single output, I was told my tx was over the size limit and I’d have to pay a fee. But it was only 227 bytes. Not sure what’s going on there.

My address book has a new address in it, “Refund from bitcoincore.org”. Nice! Perhaps those addresses should be treated like change addresses and hidden unless you actually receive money to them. Otherwise the address book will end up quite cluttered.

SergioDemianLerner commented at 10:43 pm on April 26, 2013: contributor

Check my proposal “Merchant-pays-fee proposal for Bitcoin Payment Messages” in https://bitcointalk.org/index.php?topic=188695.0

It could be scheduled for the next hard-fork.

in src/qt/bitcoingui.cpp: in 326b85d81e outdated

761+                CClientUIInterface::ICON_WARNING);
762+}
763+
764+void BitcoinGUI::showPaymentACK(QString msg)
765+{
766+    message(tr("Payment received"), msg, CClientUIInterface::MODAL);

Diapolo commented at 9:09 am on June 3, 2013:

You can move away from using CClientUIInterface::MODAL, by picking one of these (if they fit your needs):

Diapolo commented at 8:50 am on July 22, 2013:

ping :)

mikehearn commented at 10:04 am on June 5, 2013: contributor

What’s the next step for this - another review pass? It seems my previous commit comments vanished, not sure if they were addressed or not.

gavinandresen commented at 1:59 pm on June 5, 2013: contributor

@mikehearn : I fixed the bug you found (handling multiple pay-to addresses).

Next steps are:

Do the bitcoin-qt-handles-payment-request-mime-type thing on Linux
Finish writing a test plan
Bribe some people to test on windows/linux/osx
Assuming successful testing, merge into master
Turn the gist document into BIPs

mikehearn commented at 12:12 pm on June 13, 2013: contributor

BTW Gavin, shouldn’t you create a separate address+cert for your test server? Otherwise you might find people signing payment requests as yourself or bitcoincore.org ….

gavinandresen commented at 2:09 pm on June 29, 2013: contributor

Status update:

Works for me on Mac, Linux (fixed bitcoin: URI handling for gnome) and Windows (figured out how to compile a static Qt that does not expect to dynamically load openssl).

There is a showstopper bug on Windows– I’m seeing crashes on exit (looks like another global destructor being called after exit() issue).

Still needs doing:

Install the static-openssl-Qt libraries in the pull-tester environment
Improve handling of refund addresses; creating a new, labelled, appears-in-the-address-book refund address for every payment protocol transaction is bad.

mikehearn commented at 4:15 pm on June 29, 2013: contributor

Couple of other things:

Needs a rebase, at least fTestNet -> TestNet(), but better, a new CChainParams field with the payment request protocol code that is expected.
Tor users will be surprised that payment submissions don’t go via Tor due to the missing net manager proxy code.

gavinandresen commented at 7:52 am on July 18, 2013: contributor

Loose ends tied up:

-proxy settings are used for payment requests, so Tor users don’t reveal their IP addresses. Tested by running over Tor and watching the IP addresses in the ssl_access.log on the server.

Refund addresses are not shown in the GUI (unless you receive a refund, in which case they will be properly labelled). I added a backwards-compatible change to the wallet; “purpose<address>” entries are written to wallet.dat, with values of “send” “receive” “refund” or “unknown” (“unknown” the default, and all old address book entries will be “unknown”). This is backwards-compatible because old code just ignores keys it doesn’t recognize in wallet.dat (old versions of Bitcoin will show refund addresses as “receive” addresses).

One last TODO: fix the pull-tester Windows/mingw Qt libraries so they’re statically compiled with OpenSSL, so Windows binaries from the pull-tester machine will work properly.

luke-jr commented at 6:17 pm on July 22, 2013: member

Gitian build error with protobuf:

 0+ ./configure --enable-shared=no --disable-dependency-tracking --with-protoc=/home/ubuntu/build/protobuf-2.5.0/host/protoc CXX=i586-mingw32msvc-g++ CC=i586-mingw32msvc-gcc CXXFLAGS=-frandom-seed=11 AR=i586-mingw32msvc-ar STRIP=i586-mingw32msvc-strip RANLIB=i586-mingw32msvc-ranlib OBJDUMP=i586-mingw32msvc-objdump LD=i586-mingw32msvc-ld
 1checking whether to enable maintainer-specific portions of Makefiles... yes
 2checking build system type... i686-pc-linux-gnu
 3checking host system type... i686-pc-linux-gnu
 4checking target system type... i686-pc-linux-gnu
 5checking for a BSD-compatible install... /usr/bin/install -c
 6checking whether build environment is sane... yes
 7checking for a thread-safe mkdir -p... /bin/mkdir -p
 8checking for gawk... no
 9checking for mawk... mawk
10checking whether make sets $(MAKE)... yes
11checking for gcc... i586-mingw32msvc-gcc
12checking whether the C compiler works... yes
13checking for C compiler default output file name... a.exe
14checking for suffix of executables... .exe
15checking whether we are cross compiling... configure: error: in `/home/ubuntu/build/protobuf-2.5.0':
16configure: error: cannot run C compiled programs.
17If you meant to cross compile, use `--host'.
18See `config.log' for more details

luke-jr commented at 6:37 pm on July 22, 2013: member

http://codepad.org/z5wTOeGZ gets protobuf building.

gavinandresen commented at 11:05 pm on July 22, 2013: contributor

Thanks, Luke.

Status: I’ve been setting up a debug environment on a Windows 7 machine to figure out why jenkins binaries aren’t working properly (signed payment requests are being treated as unsigned).

mikehearn commented at 6:35 pm on July 23, 2013: contributor

I don’t think it’s important for v1, but here are some notes on how to do EV cert matching (so the user sees “MtGox Co Ltd. [JP]” as with Chrome instead of mtgox.com).

An EV cert is identified by the contents of the “Certificate Policies” field in the X.509 cert. Each issuer sets the value to be different, unfortunately. The values are OIDs. I think just checking against a hard coded list is sufficient - CAs should not sign certificates that contain other issuers OIDs.

There is a list of OIDs considered valid for marking EV certs here:

https://certs.opera.com/03/ev-oids.xml

(also, on Wikipedia).

I believe just having an array in the source would be sufficient to verify this. When you see the magic marker, it’s OK to use the O field instead of the CN field of the subject, giving a friendly name instead of a domain name.

in doc/readme-qt.md: in b8c36c8031 outdated

47@@ -48,7 +48,7 @@ An executable named `bitcoin-qt` will be built.
48 * Execute the following commands in a terminal to get the dependencies using MacPorts
49 
50 		sudo port selfupdate
51-		sudo port install boost db48 miniupnpc
52+		sudo port install boost db48 miniupnpc protobuf-cpp
53 
54 * Execute the following commands in a terminal to get the dependencies using HomeBrew:
55

fanquake commented at 2:15 am on July 26, 2013:

You could also add the new HomeBrew dependancy here. It’s protobuf

gavinandresen commented at 5:12 am on July 27, 2013: contributor

Note to self: secure requests showing as insecure on Windows7 is this Qt bug, I believe: https://bugreports.qt-project.org/browse/QTBUG-24827

See QWindowsCaRootFetcher class in qtbase/src/network/ssl/qsslsocket_openssl.cpp in the Qt5 source code for the fix; if we’re going to move to Qt5 soon then I would rather not add an #if QT_VERSION < 0x050000 variant on that code.

mikehearn commented at 9:56 am on July 29, 2013: contributor

I think a better fix would be to go with the original suggestion of shipping a set of trusted root CAs with the app. Otherwise this kind of random platform inconsistency risks undermining the whole initiative. Upgrading to Qt5 simply in order to work with just-in-time root cert downloads sounds like a big effort.

Diapolo commented at 10:16 am on July 29, 2013: none

@mikehearn Our Qt code is Qt5.1 compatible, should be no problem to upgrade to Qt5 code-wise, but “only” Gitian wise IMHO.

mikehearn commented at 10:36 am on July 29, 2013: contributor

If Qt5 didn’t have any API breaks, then sure, why not do the upgrade. But it doesn’t change my view that we should be shipping a set of root certs. It’s crazy not to - otherwise the first thing that will happen is people who try and use this will discover that some random subset of wallet apps can’t verify their payment requests, and someone else will have to do a long and painful process of manually intersecting the root cert set for every platform where Bitcoin wallets might run. Then that manually intersected set will become the canonical root CA cert set.

Gavin can avoid all that pain and misery by just generating his own set of root CAs and shipping them. It will never make sense to rely on the OS provided stores, IMO.

gavinandresen commented at 3:45 am on July 30, 2013: contributor

I really don’t want to take responsibility for keeping an up-to-date list of root certificates that all bitcoin wallet implementations are encouraged to support.

And having a different set of root certificates supported in the user’s web browser and bitcoin wallet seems like a really bad idea, too– “what do you mean the payment request from foo.com is insecure, I GOT IT DIRECTLY FROM THE WEB PAGE AT https://foo.com/ AND GOT THE GREEN PADLOCK !!!!”

luke-jr commented at 4:43 am on July 30, 2013: member

@gavinandresen Well, in this case, as long as the browser and wallet are using the OS’s cert store (even with the Microsoft-downloaded root certs), we can be sure that if the user went to https://foo.com, he also has the cert for it. I agree that software (including browsers, sorry @mikehearn) has no business overriding/ignoring the OS’s cert store and using their own.

mikehearn commented at 8:32 am on July 30, 2013: contributor

I already spelled out the alternative - merchants will just have to do the same work instead, and you’ll get the same situation with browsers where people rely on apocryphal and unverifiable claims like “our certs have a 94% acceptance rate”. Also, in the absence of guidance hardware devices like the Trezor will still have to make up their own lists, and those may or may not usefully overlap with peoples platform of choice. So someone will have to make and maintain a list. It might as well be standardised upstream and save everyone redundant work and pain.

This really isn’t hard. Use the list Firefox ships with, done:

http://www.mozilla.org/projects/security/certs/included/

gavinandresen commented at 6:03 am on July 31, 2013: contributor

Merged with @laanwj ’s changes so it compiles with Qt 5.

I think this is ready to be pulled; gitian changes to compile releases with Qt5 can happen after merge.

in bitcoin-qt.pro: in f5fbcdcc6b outdated

20-#    BDB_LIB_PATH, OPENSSL_INCLUDE_PATH and OPENSSL_LIB_PATH respectively
21+#    BOOST_INCLUDE_PATH BOOST_LIB_PATH,
22+#    BDB_INCLUDE_PATH BDB_LIB_PATH,
23+#    OPENSSL_INCLUDE_PATH OPENSSL_LIB_PATH
24+#    PROTOBUF_INCLUDE_PATH PROTOBUF_LIB_PATH
25+#    PROTOC : protocol buffer compiler tool

luke-jr commented at 6:42 am on August 2, 2013:

This mixes space-delimited and comma-delimited.

in contrib/gitian-descriptors/gitian.yml: in f5fbcdcc6b outdated

45   cd ..
46   #
47+  tar xjfm protobuf-2.5.0.tar.bz2
48+  cd protobuf-2.5.0
49+  ./configure --prefix=$INSTDIR --enable-static --disable-shared
50+  make $MAKEOPTS install

luke-jr commented at 6:44 am on August 2, 2013:

Will Ubuntu’s protobuf 2.2 packages not work?

luke-jr commented at 7:05 am on August 2, 2013: member

Suggestions from testplan:

Make Bitcoin-Qt visible when a URI is opened (it just remains hidden or in the background at present; this bug is in master already)
Allow user to add a label to signed payments.
Allow cancelling/removing signed payment requests individually (without Clear All).
Italicise or otherwise make the merchant name stand out in send confirmation dialog.
Store and show memo (possibly overridden by optional label) and merchant information in transaction list/details.
Store and make available the signed request, so you can prove the merchant authorized it.
Show a progress or waiting status after confirmation, until response is received.
Expired payment requests should do something (I got no result at all).
If multiple signed payment requests attempt to use the same address, the user gets an error about duplicate addresses despite never seeing the address. This message should be improved.
PaymentACK messages need to be escaped.
Multiple addresses in a single payment request error - possibly related to #1850

I also skimmed over the code, and merged it with a bunch of other pullrequests (for next-test). Nothing stood out as obviously wrong or odd.

Diapolo commented at 7:44 am on August 2, 2013: none

@luke-jr

Make Bitcoin-Qt visible when a URI is opened (it just remains hidden or in the background at present; this bug is in master already)

That is a bug, which seems to be introduced by the GUI refactoring and was not spotted. I’m looking into this.

gavinandresen commented at 9:42 am on August 2, 2013: contributor

Expired payment requests: agreed, user should ALWAYS get some result when clicking on a URI link. Escaping PaymentACK messages: nice catch, I’ll fix. Multiple addresses is a regression I’ll fix again.

All the rest: improvements that I think should happen after pulling. Some of them (like where/how to story PaymetnRequests) should probably wait until after other high priorities like reimplementing the wallet code.

Diapolo commented at 12:16 pm on August 2, 2013: none

#2872 contains the fix, which currently prevents the main window to showup after clicking a valid bitcoin: URI.

gavinandresen commented at 6:15 am on August 3, 2013: contributor

Expired payment requests now show an error. PaymentACK messages are properly HTML-escaped. Sending to multiple addresses works properly. And I verified that it communicates through Tor if you’re running proxied through Tor.

gavinandresen commented at 1:25 am on August 8, 2013: contributor

@Diapolo : I’d guess there is an operator+(char*, QString) that produces a QString, and the QString serializer adds the double-quotes.

Couple more bug reports from a tester:

PaymentACK dialog box should say “Payment Acknowledged”, since “Received” might imply that the payment is confirmed already.
Payment requests asking the user to create dust TxOuts should be rejected right away with a message to the user; otherwise, transaction creation fails.

If transaction creation DOES fail for some reason (e.g. insufficient wallet balance), there’s a question of whether or not the payment request should be automatically cleared. I’m not sure of the right answer– maybe the user just received some bitcoins and just has to wait a few minutes for them to confirm, so leaving the payment request is the right thing to do. And it is easy enough to push the clear button on the Send tab…

luke-jr commented at 2:03 am on August 8, 2013: member

IMO “acknowledged” implies confirmation more than “received” does O.o

sipa commented at 1:34 am on August 15, 2013: member

I haven’t had the time to look through this in detail, but it seems that when processing a payment request, a normal transaction is created and sent, and when this completes, the paymentACK is fetched, without even retrying if the connection failed?

IMHO, the makes the entire second step (notifying the merchant of the transaction, with metadata, refund, memo, …) useless, as it becomes entirely unreliable. I understand you can’t always guarantee everything, but can we at least:

Not store or broadcast the transaction if the connection to the payment_uri fails.
If no paymentACK is received (but the connection did succeed, so the merchant may or may not have gotten the transaction), keep retrying to get it.

gmaxwell commented at 1:41 am on August 15, 2013: contributor

The obvious way to make sure the the back-channel had atomic reliability would be a flag in the request to only submit a transaction inside the response, not via the network. Then you could be confident that either it would be successful or the transaction would fail. (Or the client did something wrong, but a client could send funds into a black hole without the payment protocol’s help)

sipa commented at 1:45 am on August 15, 2013: member

My idea would be to store the payment request in a wallet transaction field, and give it a flag not to broadcast. Seeing the transaction on the network would remove the flag, as would receiving a paymentACK. I don’t think there is any use case for wanting the transaction broadcast before the receiver confirms receiving it (and him broadcasting it, is a form of confirming).

gavinandresen commented at 4:26 am on August 15, 2013: contributor

@sipa : “… makes the entire second step useless, as it becomes entirely unreliable” ?

In the normal course of events, the user clicks on link, their wallet fetches a payment request from the merchant’s server, and then a minute or two later (after user inspect transaction details and unlocks wallet) the Payment message is sent to the merchant’s server.

So it will only be unreliable if the merchant’s server or user’s internet connection goes down in that minute or two.

I REALLY don’t think that will happen often enough to justify the added complexity of marking transactions as “don’t broadcast/rebroadcast”, modifying the GUI to show the user that they’re “pending submission”, locking the inputs, giving the user some way of double-spending a “pending submission” transaction or automatically double-spending after some period of time with retries has passed, etc etc etc.

If I’m wrong, then I’ll write that code. But I’d really like to move on to higher priorities.

luke-jr commented at 5:35 am on August 15, 2013: member

Probably easier to implement @sipa’s suggestion after we’re able to replace transactions generally.

sipa commented at 8:26 am on August 15, 2013: member

@gavinandresen What if as a gambling site, your server goes down for a minute. If you have high traffic, you’ll have many users making bets for which they were still able to fetch the payment request. You have no way of paying them without knowing a refund address, and you have no way to contact them. Of course you can wait for them to contact you, but if that is necessary for every minute of downtime, you’ll need very high reliability of your service (DoS attacks, anyone?), or poor customer support.

It is true that the locked funds issue right now makes this harder, as we cannot deal well with non-confirming transactions. I consider that a separate issue, but it makes an optimal implementation difficult now. But if you can’t do that, please at least save the payment request, and retry getting PaymentACKs for some time, just like we retry broadcasting normal transactions.

And put a suggestion in the BIP that this is recommended. Even if you can’t implement it yourself now - in some environments it may be significantly easier to do.

gavinandresen commented at 10:50 pm on August 15, 2013: contributor

@sipa : You’re right, that use case works much better if it is “merchant broadcasts first”.

I’ll look into rebroadcasting. I suspect it will be easier to just lock the inputs for the estimated worst-case Payment–>PaymentACK round trip, and broadcast the transaction when the PaymentACK is received. If Payment->PaymentACK succeeds, then broadcast the transaction; if it fails, then just unlock the inputs and tell the user “error communicating”.

I’ve been putting off writing code to save the PaymentRequest/Payment/PaymentACK messages in the wallet, because adding more stuff to wallet.dat when we’re likely to rewrite it soon for HD support might make the upgrade harder, and because I’m not planning on implementing any GUI for looking at old PaymentRequests. But I should probably save the data anyway.

Reject dust amounts during validation

Replaces the validation check for "amount == 0" with an isDust check,
so very small output amounts are caught before the wallet
is unlocked, a transaction is created, etc.

57d80467f1

GetDataDir(): cache paths for each network separately b94595bb7f

Refactor: CAddressBookData for mapAddressBook

Straight refactor, so mapAddressBook stores a CAddressBookData
(which just contains a std::string) instead of a std::string.

Preparation for payment protocol work, which will add the notion
of refund addresses to the address book.

618855133d

Refactor: Move GetAccountAddresses to CWallet 3624356e82

update SelectParamsFromCommandLine() handling/order

- move SelectParamsFromCommandLine() from init.cpp to bitcoin.cpp to allow
  to use TestNet() for Bitcoin-Qt instead of GetBoolArg("-testnet", false)
- change order in bitcoind.cpp to match bitcoin.cpp functionality
- hamonize error message strings for missing datadir and failing
  SelectParamsFromCommandLine() in bitcoin.cpp and bitcoind.cpp
- use TestNet() call in splashscreen.cpp

a2189fbaf6

Rework when payment server is started a73aa68b84

Route qDebug() messages to debug.log 47d0534368

gavinandresen commented at 3:55 am on August 22, 2013: contributor

Rebased (needed to fix conflict in bitcoin-qt.pro).

And made a functionality tweak: PaymentRequests are now written to wallet.dat, using the (formerly unused and always empty) vector<pair<string,string>> vOrderForm field in CWalletTx. Each PaymentRequest satisfied by the transaction has key=“PaymentRequest” value=…serialized PaymentRequest protocol buffer message.

The transaction details information also now shows the Merchant (or merchants) associated with a transaction. @sipa: since one transaction can satisfy several PaymentRequest messages, locking the inputs until we get a PaymentACK doesn’t work; e.g. if a transaction satisfies requests from merchants A and B, we’d send Payment messages to both A and B. If one of them fails, then we can’t cancel the transaction– the other merchant will go ahead and broadcast it for us.

Rebroadcasting the Payment message if the merchant’s site goes down would be nice to have. “patches welcome”

Payment Protocol: X509-validated payment requests

Add support for a Payment Protocol to Bitcoin-Qt.

Payment messages are protocol-buffer encoded and communicated over
http(s), so this adds a dependency on the Google protocol buffer
library, and requires Qt with OpenSSL support.

a41d5fe019

BitcoinPullTester commented at 6:55 am on August 22, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a41d5fe01947f2f878c055670986a165af800f9a 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.

gavinandresen referenced this in commit e62f8d72f3 on Aug 22, 2013

gavinandresen merged this on Aug 22, 2013

gavinandresen closed this on Aug 22, 2013

mikehearn commented at 11:10 am on August 22, 2013: contributor

Congrats everyone. This is a big step forward for the Bitcoin world!

gavinandresen deleted the branch on Mar 13, 2014

rebroad commented at 4:51 am on August 28, 2016: contributor

Compiler is giving this:

0qt/test/paymentservertests.cpp: In member function ‘void PaymentServerTests::paymentServerTests()’:
1qt/test/paymentservertests.cpp:65:6: warning: stack protector not protecting local variables: variable length buffer [-Wstack-protector]
2 void PaymentServerTests::paymentServerTests()
3      ^

I suspect it’s not related to this pull…

luke-jr commented at 5:09 am on August 28, 2016: member

@rebroad As a rule, whenever you feel inclined to leave a comment on a closed issue/PR that hasn’t been touched in months, just don’t. It’s almost never the right place for discussion. Instead, if you wish to discuss a warning/error/bug, open a new issue (or better yet, fix it and open a PR).

DrahtBot locked this on Sep 8, 2021

Payment Protocol Work #2539