[Qt] Payment request expiration bug fix (re-done) #5620

• this is based on #4122 (which can be closed)

Currently a payment request is only checked for expiration upon receipt. It should be checked again immediately before sending coins to prevent the user from paying to an expired invoice which would then require a customer service interaction.

• add static verifyExpired() function to PaymentServer to be able to use the same validation code in GUI and unit-testing code
• extend unit tests to use that function and also add an unit test which overflows, because payment requests allow expires as uint64, whereas we use int64_t for verification of expired payment requests

Looks good to me, utACK

@Diapolo would it be possible to cover this with a test in paymentservertests.cpp?

One could craft and add an expired payment request into paymentrequestdata.h, copy and paste the check used in this pull (details.has_expires() && (int64_t)details.expires() < GetTime())) and use that in a testcase, yes. I can't craft a usable payment request usable there, feel free to jump in :).

Btw. @laanwj expires is also an uint64 casted to int64_t, is that safe? GetTime()is using int64_t as return value, wheras the used time() in it is returning time_t (which is a long long on my compiler suite).

Thinking about it, uint64 allows for bigger numbers than int64 can store, so if one crafts payment request with expires set to a value bigger than 0x7FFFFFFFFFFFFFFF it would be considered valid for a very long time, right (if -9223372036854775807 < GetTime())? Could this become a problem?

Definitely needs a unit test (or two, testing expires with large unsigned values is a great idea). @Diapolo : if you aren't willing to create unit tests for your code, then, in my humble opinion, you shouldn't be writing code. Testing is critical.

@gavinandresen Sorry if you have the impression I'm not willing, it's more my skills are limited in that case. I really have no idea how to craft a suiting payment request for paymentrequestdata.h, like you did -_-.

Oh and by the way, you created the initial check in paymentserver.cpp, which also had no unit test :-P. I just duplicated the code which was there and added it, where it also needs to kick in!

@Diapolo i'll try to create one a link you in a commit for paymentrequestdata.h

@jonasschnelli I'm glad if you also can help me in understanding how you do it :).

There's simple c++ code for creating payment requests here: https://github.com/gavinandresen/paymentrequest/blob/master/c%2B%2B/paymentrequest-create.cpp

I used it to generate a binary payment request (after using openssl commands to generate private keys, certificates, etc).

Then I converted the binary payment request files to base64 (e.g. openssl enc -base64)

Then copy the base64 into the .h file.

@gavinandresen Thanks for linking that in here, I'll take a look.

Could you not take one from https://bitcoincore.org/~gavin/createpaymentrequest.php and base64 it? If you need different values, take the php source, run them on MAMP/XAMP, etc. I suppose this might also work.

@Diapolo ping me if you didn't succeed then i start looking at it.

@gavinandresen Wow, that was quite hard up to where I am now :). I took your whole branch https://github.com/gavinandresen/paymentrequest and converted the entire create_ca.sh to a Windows compatible create_ca.cmd, reworked build_detect_platform, Makefile, paymentrequest-create.cpp, paymentrequest-dump.cpp and even the README.txt (to perhaps aid others with this stuff) to be Windows compatible!

I build the executables with MinGW 4.9.2 x64 and the ca stuff with OpenSSL 1.0.1j.

Now when I try to do paymentrequest-create paytoaddress=1BTCorgHwCg6u2YSAWKgS17qUad6kHmtQW memo="Just Testing" amount=11.0 | paymentrequest-dump, I get the following error, which I could not solve until now:

<pre> [libprotobuf ERROR google/protobuf/message_lite.cc:123] Can't parse message of type "payments.PaymentRequest" because it is missing required fields: serialized_payment_details </pre>

Can you or any other (@mikehearn) assist with solving this!?

Edit: paymentrequest.pb.cpp, paymentrequest.pb.h and paymentrequest.proto from your repo are identiocal to the ones I have in my Bitcoin Core build dir based on current master!

Edit2: Seems to be a problem with paymentrequest-dump.exe, as I can open the payment request file from paymentrequest-create.exe in a 0.9.3 client... strange.

I depend on #5642 getting merged, before I can add another unit test for this.

@Diapolo could you not include #5642 commits in this PR and add a UT?

@jonasschnelli Yeah I could rebase this ontop of the other pull, but I'm not sure this allows for a quicker merge. The other pr requires a review of at least one other core dev I guess...

Rebased to be on top of #5642, which has 2 ACKs now.

If this passes Travis it should be ready to merge. Needs review by @gavinandresen or @laanwj.

So 8 days ago I finished this medium priority pull and no one cares to review/ACK? Are we dropping wallet support or why is no one caring about our reference implementation of the payment protocol?

I think we all care. It's just very hard to set priorities. Your work looks really good and I will do a proper test and a review.

Please be patient and give us some more days. Thanks.

Here are some fresh built binaries from my gitian-build-server (in case some likes to test but not to build): https://bitcoin.jonasschnelli.ch/pulls/5620/

Tested, reviewed ACK.