Check specific validation error in miner tests #11220

1. 
   Sjors commented at 9:47 pm on September 2, 2017: member

   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:

   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++.

2. 
   luke-jr commented at 10:21 pm on September 2, 2017: member

   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.

3. fanquake added the label Tests on Sep 2, 2017
4. 
   sdaftuar commented at 10:58 pm on September 2, 2017: member

   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.

5. 
   Sjors commented at 7:09 am on September 3, 2017: member

   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.

6. 
   Sjors commented at 12:15 pm on September 3, 2017: member

   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:

   0BOOST_CHECK_EXCEPTION(
   1    functionThatThrows(),
   2    std::runtime_error,
   3    CheckRejectInvalid(REJECT_INVALID_BAD_CB_MULTIPLE)
   4);
   

   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.

7. 
   Sjors commented at 3:22 pm on September 3, 2017: member

   @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.
8. in src/test/testutil.h:23 in 49a7fe240a outdated

   18+#define REJECT_INVALID_BAD_CB_MULTIPLE "bad-cb-multiple"
   19+#define REJECT_INVALID_BAD_TXNS_INPUTS_MISSINGORSPENT "bad-txns-inputs-missingorspent"
   20+#define REJECT_INVALID_BLOCK_VALIDATION_FAILED "block-validation-failed"
   21+
   22+// BOOST_CHECK_THROW predicates to check the specific validation error
   23+class CheckRejectInvalid {
   

   ---

   ---

   

   MarcoFalke commented at 11:23 pm on September 5, 2017:

   Maybe move all of this to the cpp?
9. 
   Sjors commented at 8:30 am on September 6, 2017: member

   @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.

10. 
    MarcoFalke commented at 7:10 pm on October 2, 2017: member

    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
11. Sjors force-pushed on Oct 3, 2017
12. Sjors force-pushed on Oct 3, 2017
13. 
    Sjors commented at 9:43 am on October 3, 2017: member

    Done, as well as rebased to latest master.
14. Sjors force-pushed on Oct 3, 2017
15. 
    Sjors commented at 9:47 am on October 3, 2017: member

    I pushed again to fix commit message and code comment (it was saying “BOOST_CHECK_THROW” instead of “BOOST_CHECK_EXCEPTION” in two places).
16. 
    Sjors commented at 10:19 am on October 3, 2017: member

    The p2p-segwit.py test failed in one environment. Random time-out? All machines passed on my previous force push.
17. 
    meshcollider commented at 11:04 am on October 3, 2017: contributor

    @Sjors probably just an unrelated failure, Travis does that sometimes. I’ve restarted that test for you.
18. in src/test/miner_tests.cpp:31 in ddc20fdf24 outdated

    26@@ -26,6 +27,25 @@
    27 
    28 BOOST_FIXTURE_TEST_SUITE(miner_tests, TestingSetup)
    29 
    30+// Validation errors used in tests (prevent typos and facilitate refactoring):
    31+#define REJECT_INVALID_BAD_BLK_SIGOPS "bad-blk-sigops"
    

    ---

    ---

    

    promag commented at 12:36 pm on October 3, 2017:

    IMO inline these strings.

    ---

    

    Sjors commented at 1:00 pm on October 3, 2017:

    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.

    ---

    

    promag commented at 1:52 pm on October 3, 2017:

    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.

    ---

    

    Sjors commented at 11:45 am on October 4, 2017:

    Using inline now.
19. in src/test/miner_tests.cpp:20 in ddc20fdf24 outdated

    16@@ -17,6 +17,7 @@
    17 #include "uint256.h"
    18 #include "util.h"
    19 #include "utilstrencodings.h"
    20+#include <boost/algorithm/string.hpp>
    

    ---

    ---

    

    promag commented at 12:37 pm on October 3, 2017:

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

    ---

    

    Sjors commented at 11:45 am on October 4, 2017:

    I got rid of this dependency.
20. in src/test/miner_tests.cpp:39 in ddc20fdf24 outdated

    34+#define REJECT_INVALID_BLOCK_VALIDATION_FAILED "block-validation-failed"
    35+
    36+// BOOST_CHECK_EXCEPTION predicates to check the specific validation error
    37+class CheckRejectInvalid {
    38+public:
    39+    CheckRejectInvalid(std::string const& arg) {
    

    ---

    ---

    

    promag commented at 12:38 pm on October 3, 2017:

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

    ---

    

    Sjors commented at 1:04 pm on October 3, 2017:

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

    ---

    

    Sjors commented at 11:45 am on October 4, 2017:

    Done, and renamed the private variable from reason to m_reason
21. in src/test/miner_tests.cpp:43 in ddc20fdf24 outdated

    38+public:
    39+    CheckRejectInvalid(std::string const& arg) {
    40+        reason = arg;
    41+    };
    42+    bool operator() (std::runtime_error const& e) const {
    43+        return boost::contains(e.what(), reason);
    

    ---

    ---

    

    promag commented at 12:42 pm on October 3, 2017:

    Use std::string::find?

    ---

    

    Sjors commented at 12:56 pm on October 3, 2017:

    That should indeed help with #8670.

    ---

    

    Sjors commented at 11:46 am on October 4, 2017:

    Done
22. in src/test/miner_tests.cpp:42 in ddc20fdf24 outdated

    37+class CheckRejectInvalid {
    38+public:
    39+    CheckRejectInvalid(std::string const& arg) {
    40+        reason = arg;
    41+    };
    42+    bool operator() (std::runtime_error const& e) const {
    

    ---

    ---

    

    promag commented at 12:42 pm on October 3, 2017:

    Nit, const std::runtime_error& e.

    ---

    

    Sjors commented at 11:46 am on October 4, 2017:

    Done
23. in src/test/miner_tests.cpp:289 in ddc20fdf24 outdated

    283@@ -264,7 +284,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
    284         mempool.addUnchecked(hash, entry.Fee(LOWFEE).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx));
    285         tx.vin[0].prevout.hash = hash;
    286     }
    287-    BOOST_CHECK_THROW(AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey), std::runtime_error);
    288+
    289+    // This should throw bad-blk-sigops
    290+    BOOST_CHECK_EXCEPTION(AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey), std::runtime_error, CheckRejectInvalid(REJECT_INVALID_BAD_BLK_SIGOPS));
    

    ---

    ---

    

    promag commented at 12:48 pm on October 3, 2017:

    IMO this sounds better:

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

    ---

    

    Sjors commented at 12:57 pm on October 3, 2017:

    I renamed CheckRejectInvalid to HasReason; seems more readable indeed.
24. 
    promag commented at 12:49 pm on October 3, 2017: member

    Agree with better testing the exception.
25. in src/test/miner_tests.cpp:46 in ddc20fdf24 outdated

    41+    };
    42+    bool operator() (std::runtime_error const& e) const {
    43+        return boost::contains(e.what(), reason);
    44+    };
    45+private:
    46+    std::string reason;
    

    ---

    ---

    

    MarcoFalke commented at 12:58 pm on October 3, 2017:

    µnit: Can be const

    ---

    

    Sjors commented at 11:47 am on October 4, 2017:

    Done (Github needs a check-mark emoticon)
26. Sjors force-pushed on Oct 4, 2017
27. Sjors force-pushed on Oct 4, 2017
28. 
    Sjors commented at 11:50 am on October 4, 2017: member

    I believe I addressed all code style issued raised. Rebased to latest master. I also signed the commit this time.
29. 
    Sjors commented at 11:00 am on October 20, 2017: member

    @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.
30. 
    JeremyRubin commented at 10:14 pm on October 20, 2017: 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.

31. 
    Sjors commented at 5:41 am on October 21, 2017: member

    @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.
32. 
    sipa commented at 2:10 pm on October 21, 2017: member

    Concept ACK
33. 
    MarcoFalke commented at 9:19 am on October 23, 2017: member

    utACK 59d0ede514f7754ad4df11ffe11f27e2ea63bf3e
34. [Tests] check specific validation error in miner tests

    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.

    12781db058
35. Sjors force-pushed on Nov 9, 2017
36. 
    Sjors commented at 11:20 am on November 9, 2017: member

    Rebased to account for #11389.
37. laanwj merged this on Dec 19, 2017
38. laanwj closed this on Dec 19, 2017

    ---

39. laanwj referenced this in commit 2971fd030f on Dec 19, 2017
40. Sjors deleted the branch on Dec 19, 2017
41. jasonbcox referenced this in commit f7bae40b39 on Sep 27, 2019
42. PastaPastaPasta referenced this in commit 171e55f69f on Feb 13, 2020
43. PastaPastaPasta referenced this in commit d455b4499f on Feb 27, 2020
44. PastaPastaPasta referenced this in commit 5e294ba75c on Feb 27, 2020
45. PastaPastaPasta referenced this in commit 46976a50fc on Feb 27, 2020
46. PastaPastaPasta referenced this in commit 25f068f687 on Feb 27, 2020
47. PastaPastaPasta referenced this in commit b84a23fe77 on Mar 14, 2020
48. PastaPastaPasta referenced this in commit bc27db8e4d on Mar 14, 2020
49. PastaPastaPasta referenced this in commit b62cf5950b on Apr 4, 2020
50. PastaPastaPasta referenced this in commit b25eec73ed on Apr 5, 2020
51. ckti referenced this in commit c8822a4128 on Mar 28, 2021
52. DrahtBot locked this on Sep 8, 2021

Contributors
Sjors luke-jr sdaftuar MarcoFalke meshcollider promag JeremyRubin sipa

Labels
Tests