Payment Protocol Work #2539
Payment Protocol Work #2539
Conversation
|
Nice, I'll do some testing with it over the weekend. |
|
Pull-tester failure is because I didn't update the unit test data when I changed the PaymentRequest protocol buffer format... |
|
Unit tests fixed, but I bet the mingw-Windows build will not work because we'll need a mingw-compiled -lprotobuf |
|
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: 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: 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. However, this doesn't solve the problem. It changes the command line to: So PWD is an absolute path; not the relative path as used for the source files. The only way I could solve this is by changing the line in This is pretty much a hack but heh... |
|
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. |
|
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. |
|
|
||
| void BitcoinGUI::showPaymentACK(QString msg) | ||
| { | ||
| message(tr("Payment received"), msg, CClientUIInterface::MODAL); |
There was a problem hiding this comment.
You can move away from using CClientUIInterface::MODAL, by picking one of these (if they fit your needs):
CClientUIInterface::MSG_INFORMATION is ICON_INFORMATION, CClientUIInterface::MSG_WARNING is (ICON_WARNING | BTN_OK | MODAL), CClientUIInterface::MSG_ERROR is (ICON_ERROR | BTN_OK | MODAL)
|
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. |
|
@mikehearn : I fixed the bug you found (handling multiple pay-to addresses). Next steps are:
|
|
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 .... |
|
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:
|
|
Couple of other things:
|
|
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. |
|
Gitian build error with protobuf: |
|
http://codepad.org/z5wTOeGZ gets protobuf building. |
|
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). |
|
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. |
| @@ -48,7 +48,7 @@ An executable named `bitcoin-qt` will be built. | |||
| * Execute the following commands in a terminal to get the dependencies using MacPorts | |||
|
|
|||
| sudo port selfupdate | |||
| sudo port install boost db48 miniupnpc | |||
| sudo port install boost db48 miniupnpc protobuf-cpp | |||
|
|
|||
| * Execute the following commands in a terminal to get the dependencies using HomeBrew: | |||
|
|
|||
There was a problem hiding this comment.
You could also add the new HomeBrew dependancy here. It's protobuf
|
Note to self: secure requests showing as insecure on Windows7 is this Qt bug, I believe: 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. |
|
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. |
|
@mikehearn Our Qt code is Qt5.1 compatible, should be no problem to upgrade to Qt5 code-wise, but "only" Gitian wise IMHO. |
|
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. |
|
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 !!!!" |
|
@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. |
|
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: |
|
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. |
| # BDB_INCLUDE_PATH BDB_LIB_PATH, | ||
| # OPENSSL_INCLUDE_PATH OPENSSL_LIB_PATH | ||
| # PROTOBUF_INCLUDE_PATH PROTOBUF_LIB_PATH | ||
| # PROTOC : protocol buffer compiler tool |
There was a problem hiding this comment.
This mixes space-delimited and comma-delimited.
|
Suggestions from testplan:
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. |
|
Expired payment requests: agreed, user should ALWAYS get some result when clicking on a URI link. 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. |
|
#2872 contains the fix, which currently prevents the main window to showup after clicking a valid bitcoin: URI. |
|
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. |
|
@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:
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... |
|
IMO "acknowledged" implies confirmation more than "received" does O.o |
|
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:
|
|
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) |
|
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). |
|
@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. |
|
Probably easier to implement @sipa's suggestion after we're able to replace transactions generally. |
|
@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. |
|
@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. |
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.
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.
- 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
|
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" |
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.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a41d5fe01947f2f878c055670986a165af800f9a for binaries and test log. |
|
Congrats everyone. This is a big step forward for the Bitcoin world! |
|
Compiler is giving this: I suspect it's not related to this pull... |
|
@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). |
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)