[Qt] add verifySize() function to PaymentServer #5665

Diapolo commented at 8:17 AM on January 15, 2015: none

add static verifySize() function to PaymentServer and move the logging on error into the function
the function checks if the size is allowed as per BIP70

laanwj added the label GUI on Jan 15, 2015

jonasschnelli commented at 8:31 AM on January 29, 2015: contributor

I would also strongly recommend to add a Unit-Test for verifySize(). We should no longer add implementations without test-coverage. And i assume this is easy to test with another payment request in the fixtures.

Diapolo commented at 6:58 AM on January 30, 2015: none

@jonasschnelli As verifySize() IS used in PaymentServer::readPaymentRequestFromFile() we already have test coverage :). IMHO it doesn't make sense to spam paymentrequestdata.h with 50001 Bytes of garbage just to re-test verifySize() again. What do you think?

jonasschnelli commented at 9:07 AM on January 30, 2015: contributor

@Diapolo IMO we should add the 50001 bytes test. You might use this forged payment request http://jonasschnelli.ch/r1422608371.bitcoinpaymentrequest

I also tried to test PaymentServer::readPaymentRequestFromFile with 50001 random bytes. But there is no distinction between parsing error and size error (returns only a bool). Is there a change to test the QWarning within a QT unit test? Just a little string comparison?

Diapolo commented at 9:12 AM on January 30, 2015: none

@jonasschnelli I'm fine with directly using verifySize() instead of PaymentServer::readPaymentRequestFromFile() (which can then be private again ^^). But we don't need a real invalid playment request for this check IMHO. As the check is triggerd by just if > 50000 bytes rule it can be random garbage data.

What do you mean by Is there a change to test the QWarning within a QT unit test? Just a little string comparison??

jonasschnelli commented at 9:13 AM on January 30, 2015: contributor

Tested, reviewed, stepped ACK.

nit: misses unit test

bildschirmfoto 2015-01-30 um 10 10 42

jonasschnelli commented at 9:15 AM on January 30, 2015: contributor

@Diapolo: sounds even better. I would recommend to just pass 50001 bytes through PaymentServer::verifySize. I understand that the test looks very trivial know. But the idea of test are that they give guidelines when extending the implementation.

Diapolo commented at 9:25 AM on January 30, 2015: none

@jonasschnelli Updated, I hope this is fine now?

jonasschnelli commented at 9:33 AM on January 30, 2015: contributor

tested ACK

Diapolo commented at 9:48 PM on February 3, 2015: none

Anything left to do for me to get this and other related pulls merged ;)?

Diapolo commented at 7:56 AM on February 26, 2015: none

Ping

Diapolo commented at 10:05 AM on March 9, 2015: none

@laanwj Mind giving this the final ACK ;)?

jonasschnelli commented at 12:33 PM on March 11, 2015: contributor

ReACK

Diapolo commented at 5:29 PM on March 21, 2015: none

@laanwj ping

in src/qt/test/paymentservertests.cpp:None in cf6d30138f outdated

 184 | @@ -185,7 +185,8 @@ void PaymentServerTests::paymentServerTests()
 185 |      tempFile.open();
 186 |      tempFile.write((const char*)randData, sizeof(randData));
 187 |      tempFile.close();
 188 | -    QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);

laanwj commented at 8:25 AM on March 24, 2015:

Why are you removing this test?

Diapolo commented at 6:14 PM on March 24, 2015:

To be able to make readPaymentRequestFromFile private again, which I changed once and wasn't happy with it :). The function could also fail, if the file failed to open, which isn't what this test is for anyway.

laanwj commented at 8:13 AM on April 13, 2015:

If you want to be able to test a private methods you could use a friend class / function. E.g. inside the class do

friend class PaymentServerTester;

Then do the testing in a class PaymentServerTester.

Diapolo commented at 8:48 AM on May 11, 2015:

@laanwj It's not necessary here, but thanks for that C++ lesson :).

Diapolo commented at 8:39 PM on April 12, 2015: none

@jonasschnelli Maybe you can help me understand what's preventing this pull from getting merged?

Diapolo commented at 1:24 PM on May 31, 2015: none

@laanwj @jonasschnelli Ping

laanwj commented at 6:59 AM on June 4, 2015: member

@diapolo because you removed the test for, unrelated, PaymentServer::readPaymentRequestFromFile

Diapolo commented at 10:13 AM on June 5, 2015: none

@laanwj As I said, I just made PaymentServer::readPaymentRequestFromFile() public, because I hacked the former unit-test. That function isn't reliable as unit-test because it can also fail if the file could not be opened (returns false then), which isn't what the test assumes, as we want to verify the DoS protection.

jgarzik commented at 6:51 PM on July 23, 2015: contributor

ut ACK

Diapolo commented at 6:05 AM on July 24, 2015: none

@laanwj Fixed merge-conflict, ready now.

[Qt] add verifySize() function to PaymentServer

- add static verifySize() function to PaymentServer and move the logging
  on error into the function
- also use the new function in the unit test
- the function checks if the size is allowed as per BIP70

be942def4b

Diapolo commented at 12:13 PM on August 10, 2015: none

@laanwj Ping

Diapolo commented at 7:56 PM on September 9, 2015: none

Final ping!

jgarzik merged this on Sep 15, 2015

jgarzik closed this on Sep 15, 2015

jgarzik referenced this in commit 6f55cddf6b on Sep 15, 2015

Diapolo deleted the branch on Sep 15, 2015

MarcoFalke locked this on Sep 8, 2021