Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 15, 2015
Merged

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

merged 1 commit into from Sep 15, 2015

Conversation

Diapolo
Copy link

@Diapolo Diapolo commented Jan 15, 2015

  • 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 laanwj added the GUI label Jan 15, 2015
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jan 29, 2015

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
Copy link
Author

Diapolo commented Jan 30, 2015

@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
Copy link
Contributor

jonasschnelli commented Jan 30, 2015

@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
Copy link
Author

Diapolo commented Jan 30, 2015

@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
Copy link
Contributor

jonasschnelli commented Jan 30, 2015

Tested, reviewed, stepped ACK.

nit: misses unit test

bildschirmfoto 2015-01-30 um 10 10 42

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jan 30, 2015

@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
Copy link
Author

Diapolo commented Jan 30, 2015

@jonasschnelli Updated, I hope this is fine now?

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jan 30, 2015

tested ACK

@Diapolo
Copy link
Author

Diapolo commented Feb 3, 2015

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

@Diapolo
Copy link
Author

Diapolo commented Feb 26, 2015

Ping

@Diapolo
Copy link
Author

Diapolo commented Mar 9, 2015

@laanwj Mind giving this the final ACK ;)?

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Mar 11, 2015

ReACK

@Diapolo
Copy link
Author

Diapolo commented Mar 21, 2015

@laanwj ping

@@ -185,7 +185,8 @@ void PaymentServerTests::paymentServerTests()
tempFile.open();
tempFile.write((const char*)randData, sizeof(randData));
tempFile.close();
QCOMPARE(PaymentServer::readPaymentRequestFromFile(tempFile.fileName(), r.paymentRequest), false);
Copy link
Member

@laanwj laanwj Mar 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing this test?

Copy link
Author

@Diapolo Diapolo Mar 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@laanwj laanwj Apr 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Diapolo Diapolo May 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Diapolo
Copy link
Author

Diapolo commented Apr 12, 2015

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

@Diapolo
Copy link
Author

Diapolo commented May 31, 2015

@laanwj @jonasschnelli Ping

@laanwj
Copy link
Member

laanwj commented Jun 4, 2015

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

@Diapolo
Copy link
Author

Diapolo commented Jun 5, 2015

@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
Copy link
Contributor

jgarzik commented Jul 23, 2015

ut ACK

@Diapolo
Copy link
Author

Diapolo commented Jul 24, 2015

@laanwj Fixed merge-conflict, ready now.

- 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
@Diapolo
Copy link
Author

Diapolo commented Aug 10, 2015

@laanwj Ping

@Diapolo
Copy link
Author

Diapolo commented Sep 9, 2015

Final ping!

@jgarzik jgarzik merged commit be942de into bitcoin:master Sep 15, 2015
1 check passed
jgarzik pushed a commit that referenced this issue Sep 15, 2015
@Diapolo Diapolo deleted the pr_size branch Sep 15, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants