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

Check specific validation error in miner tests #11220

Merged

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Sep 2, 2017

Problem

BOOST_CHECK_THROW merely checks that some std::runtime_error is
thrown, but not which one.

Here's an example of how this can cause a test to pass when a developer
introduces a consensus bug. The test for the sigops limit assumes
that CreateNewBlock fails with bad-blk-sigops. However it can
also fail with bad-txns-vout-negative, if a naive developer lowers
BLOCKSUBSIDY to 1*COIN.

Solution

BOOST_CHECK_EXCEPTION allows an additional predicate function. This
commit uses this for all exceptions that are checked for in
miner_tets.cpp:

  • bad-blk-sigops
  • bad-cb-multiple
  • bad-txns-inputs-missingorspent
  • block-validation-failed

If the function throws a different error, the test will fail. Although the message produced by Boost is a bit confusing, it does show which error was actually thrown. Here's what the above 1*COIN bug would result in:

schermafbeelding 2017-09-02 om 23 42 29

Other considerations

A more elegant solution in my opinion would be to subclass std::runtime_error for each INVALID_TRANSACTION type, but this would involve touching consensus code.

I put the predicates in test_bitcoin.h because I assume they can be reused in other test files. However serialize_tests.cpp also uses BOOST_CHECK_EXCEPTION and it defines the predicate in the test file itself.

Instead of four IsRejectInvalidReasonX(std::runtime_error const& e) functions, I'd prefer something reusable like bool IsRejectInvalidReason(String reason)(std::runtime_error const& e), which would be used like BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops"). I couldn't figure out how to do that in C++.

@luke-jr
Copy link
Member

luke-jr commented Sep 2, 2017

Most of the rejection reasons are not standardised, and we only care that it fails. It might make the most sense to check that it's a consensus_rule_violation exception or such, but I'm not sure checking the specific string is useful.

Your example is invalid, because for the bad-blk-sigops test, we would not care what the outcome is if the subsidy is broken. Instead, the subsidy should be tested separately, not implicitly.

@fanquake fanquake added the Tests label Sep 2, 2017
@sdaftuar
Copy link
Member

sdaftuar commented Sep 2, 2017

Concept ACK. I think in general checking for the specific error we expect makes it less likely that we inadvertently introduce a bug in the tests; certainly this kind of thing has cropped up in the functional test suite, where bugs introduced into a test harness cause the test to not actually be testing anything, or to be testing the wrong thing.

IMO the downside (in general) to additional specificity is that it adds overhead when making changes to the consensus code rejection reasons, because now all the tests that were hardcoded for a particular reason need updating. This patch seems pretty small though, and I don't think that should be much of a concern here. In fact I think we largely already do similar reject-reason-pattern-matching in the functional tests as well.

@Sjors
Copy link
Member Author

Sjors commented Sep 3, 2017

I'll see what I can do about Travis failing on everything but OSX. It's throwing multiple definition of IsRejectInvalidReason in a bunch of places.

I agree the example isn't great. I did actually run into it while trying an extremely high value for MAX_BLOCK_SIGOPS_COST (250x) in an experiment. In attempt to make the test pass, I increased the transaction generation loops from 1001 to 250001, which obviously(?) took forever to run. I then tried fewer loops to get a sense of performance. To my surprise the test passed for some seemingly random intermediate value. Once I found the actual error message I realized it was an error in the modified test; it just ran out of coins somewhere in the loop. Lowering the coinbase reward in the test was an easier way to reproduce that specific error.

@Sjors
Copy link
Member Author

Sjors commented Sep 3, 2017

I pushed a few improvements, apparently to the satisfaction of Travis:

  1. moved changes from test_bitcoin.h to testutil.h, which seems more appropriate
  2. instead of one function for each validation error type, there's now a class CheckRejectInvalid whose constructor takes a string
  3. #define for validation error strings so they're all in one place. That should make later refactoring easier and also prevent typos.

So now each test looks like this:

BOOST_CHECK_EXCEPTION(
    functionThatThrows(),
    std::runtime_error,
    CheckRejectInvalid(REJECT_INVALID_BAD_CB_MULTIPLE)
);

Unfortunately as a result of this change, when a test throws a different exception, it no longer shows what that exception was. I don't know why. However, this is easy for a developer to debug: just put BOOST_CHECK(functionThatThrows()) above the failing BOOST_CHECK_EXCEPTION and run the test again.

@Sjors
Copy link
Member Author

Sjors commented Sep 3, 2017

@sipa and @morcos worked on miner_tests.cpp earlier this year.

@morcos since you refactored CreateNewBlock in late 2015 (#6898), could you check that I picked the right specific validation errors? Just to make sure the tests weren't already misbehaving.

#define REJECT_INVALID_BLOCK_VALIDATION_FAILED "block-validation-failed"

// BOOST_CHECK_THROW predicates to check the specific validation error
class CheckRejectInvalid {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move all of this to the cpp?

@Sjors
Copy link
Member Author

Sjors commented Sep 6, 2017

@MarcoFalke since #11234 just snatched testutil.h out from under me, I moved everything into miner_test.cpp.

There are a lot of other tests that use BOOST_CHECK_THROW without checking the precise error, so it might make sense to revert #11234 at some point.

@maflcko
Copy link
Member

maflcko commented Oct 2, 2017

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@Sjors Sjors force-pushed the miner-test-check-specific-validation-error branch 2 times, most recently from 24652c2 to 9dfaced Compare October 3, 2017 09:43
@Sjors
Copy link
Member Author

Sjors commented Oct 3, 2017

Done, as well as rebased to latest master.

@Sjors Sjors force-pushed the miner-test-check-specific-validation-error branch from 9dfaced to ddc20fd Compare October 3, 2017 09:45
@Sjors
Copy link
Member Author

Sjors commented Oct 3, 2017

I pushed again to fix commit message and code comment (it was saying "BOOST_CHECK_THROW" instead of "BOOST_CHECK_EXCEPTION" in two places).

@Sjors
Copy link
Member Author

Sjors commented Oct 3, 2017

The p2p-segwit.py test failed in one environment. Random time-out? All machines passed on my previous force push.
schermafbeelding 2017-10-03 om 11 16 49

@meshcollider
Copy link
Contributor

@Sjors probably just an unrelated failure, Travis does that sometimes. I've restarted that test for you.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Agree with better testing the exception.

@@ -26,6 +27,25 @@

BOOST_FIXTURE_TEST_SUITE(miner_tests, TestingSetup)

// Validation errors used in tests (prevent typos and facilitate refactoring):
#define REJECT_INVALID_BAD_BLK_SIGOPS "bad-blk-sigops"
Copy link
Member

Choose a reason for hiding this comment

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

IMO inline these strings.

Copy link
Member Author

@Sjors Sjors Oct 3, 2017

Choose a reason for hiding this comment

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

I used #define to make it (slightly) easier to find all occurrences in the codebase if these rejection reasons ever need to be renamed. It also helps against typos, although typos would be immediately obvious when you run the test.

Happy to change it to inline if nobody objects.

Copy link
Member

Choose a reason for hiding this comment

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

The test will fail if there is a typo. Having inline is good because a rename forces you to update the relevant tests and double check everything related.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using inline now.

@@ -17,6 +17,7 @@
#include "uint256.h"
#include "util.h"
#include "utilstrencodings.h"
#include <boost/algorithm/string.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Move down near other boost includes (try to keep it sorted too).

Copy link
Member Author

Choose a reason for hiding this comment

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

I got rid of this dependency.

// BOOST_CHECK_EXCEPTION predicates to check the specific validation error
class CheckRejectInvalid {
public:
CheckRejectInvalid(std::string const& arg) {
Copy link
Member

Choose a reason for hiding this comment

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

CheckRejectInvalid(const std::string& reason) : m_reason(reason) {}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. If it wasn't obvious, I'm not terribly familiar with C++. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, and renamed the private variable from reason to m_reason

reason = arg;
};
bool operator() (std::runtime_error const& e) const {
return boost::contains(e.what(), reason);
Copy link
Member

Choose a reason for hiding this comment

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

Use std::string::find?

Copy link
Member Author

Choose a reason for hiding this comment

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

That should indeed help with #8670.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

CheckRejectInvalid(std::string const& arg) {
reason = arg;
};
bool operator() (std::runtime_error const& e) const {
Copy link
Member

Choose a reason for hiding this comment

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

Nit, const std::runtime_error& e.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

BOOST_CHECK_THROW(AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey), std::runtime_error);

// This should throw bad-blk-sigops
BOOST_CHECK_EXCEPTION(AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey), std::runtime_error, CheckRejectInvalid(REJECT_INVALID_BAD_BLK_SIGOPS));
Copy link
Member

Choose a reason for hiding this comment

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

IMO this sounds better:

..., std::runtime_error, HasReason("bad-blk-sigops"));

Copy link
Member Author

@Sjors Sjors Oct 3, 2017

Choose a reason for hiding this comment

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

I renamed CheckRejectInvalid to HasReason; seems more readable indeed.

return boost::contains(e.what(), reason);
};
private:
std::string reason;
Copy link
Member

Choose a reason for hiding this comment

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

µnit: Can be const

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (Github needs a check-mark emoticon)

@Sjors Sjors force-pushed the miner-test-check-specific-validation-error branch 2 times, most recently from bef9996 to 59d0ede Compare October 4, 2017 11:49
@Sjors
Copy link
Member Author

Sjors commented Oct 4, 2017

I believe I addressed all code style issued raised. Rebased to latest master. I also signed the commit this time.

@Sjors
Copy link
Member Author

Sjors commented Oct 20, 2017

@JeremyRubin it would be great to get your feedback on this PR, given that you're working on these validation errors. I was able to cherry-pick my commit on top of your #11523 without a problem.

@JeremyRubin
Copy link
Contributor

Concept Ack!

I would think one improvement might be if the strings were not to be literals (as Luke points out, they aren't standardized). I think it's ok for now though.

@Sjors
Copy link
Member Author

Sjors commented Oct 21, 2017

@JeremyRubin the downside of squashing commits is you can't see history :-) I actually used #define REJECT_INVALID_BAD_CB_MULTIPLE "invalid_bad_cb_multiple" in an earlier version, but switched back to literal strings based on @promag's feedback.

@sipa
Copy link
Member

sipa commented Oct 21, 2017

Concept ACK

@maflcko
Copy link
Member

maflcko commented Oct 23, 2017

utACK 59d0ede514f7754ad4df11ffe11f27e2ea63bf3e

BOOST_CHECK_THROW merely checks that some std::runtime_error is
thrown, but not which one.

One example of how this could lead to a test passing when a developer
introduces a consensus bug: the test for the sigops limit assumes
that CreateNewBlock fails with bad-blk-sigops. However it can
also fail with bad-txns-vout-negative, e.g. if a naive developer lowers
BLOCKSUBSIDY to 1*COIN in the test.

BOOST_CHECK_EXCEPTION allows an additional predicate function. This
commit uses this for all exceptions that are checked for in
miner_tets.cpp:
* bad-blk-sigops
* bad-cb-multiple
* bad-txns-inputs-missingorspent
* block-validation-failed

An instance of the CheckRejectInvalid class (for a given validation string)
is passed to BOOST_CHECK_EXCEPTION.
@Sjors Sjors force-pushed the miner-test-check-specific-validation-error branch from 59d0ede to 12781db Compare November 9, 2017 11:03
@Sjors
Copy link
Member Author

Sjors commented Nov 9, 2017

Rebased to account for #11389.

@laanwj laanwj merged commit 12781db into bitcoin:master Dec 19, 2017
laanwj added a commit that referenced this pull request Dec 19, 2017
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
@Sjors Sjors deleted the miner-test-check-specific-validation-error branch December 19, 2017 12:53
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 27, 2019
Summary:
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1

Backport of Core PR11220
bitcoin/bitcoin#11220

Note: the exceptions we check for are slightly different and as follows:
`bad-blk-sigops`
`bad-tx-coinbase`
`bad-txns-inputs-missingorspent`
`blk-bad-inputs`

Depends on D4081

Test Plan:
  make check

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4042
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 14, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Mar 14, 2020
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 4, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 5, 2020
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## Problem

  `BOOST_CHECK_THROW` merely checks that some `std::runtime_error` is
  thrown, but not which one.

  Here's an example of how this can cause a test to pass when a developer
  introduces a consensus bug. The test for the sigops limit assumes
  that `CreateNewBlock` fails with `bad-blk-sigops`. However it can
  also fail with bad-txns-vout-negative, if a naive developer lowers
  `BLOCKSUBSIDY` to `1*COIN`.

  ## Solution

  `BOOST_CHECK_EXCEPTION` allows an additional predicate function. This
  commit uses this for all exceptions that are checked for in
  `miner_tets.cpp`:
  * `bad-blk-sigops`
  * `bad-cb-multiple`
  * `bad-txns-inputs-missingorspent`
  * `block-validation-failed`

  If the function throws a different error, the test will fail. Although the message produced by Boost is a bit [confusing](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), it does show which error was actually thrown. Here's what the above `1*COIN` bug would result in:

  <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">

  ## Other considerations

  A more elegant solution in my opinion would be to subclass `std::runtime_error` for each `INVALID_TRANSACTION` type, but this would involve touching consensus code.

  I put the predicates in `test_bitcoin.h` because I assume they can be reused in other test files. However [serialize_tests.cpp](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) also uses `BOOST_CHECK_EXCEPTION` and it defines the predicate in the test file itself.

  Instead of four `IsRejectInvalidReasonX(std::runtime_error const& e)` functions, I'd prefer something reusable like `bool IsRejectInvalidReason(String reason)(std::runtime_error const& e)`, which would be used like `BOOST_CHECK_EXCEPTION(functionThatThrows(), std::runtime_error, IsRejectInvalidReason("bad-blk-sigops")`. I couldn't figure out how to do that in C++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1
@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

10 participants