Page MenuHomePhabricator

Merge #11220: Check specific validation error in miner tests

Authored by nakihito on Sep 11 2019, 17:36.



12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

    1. 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.
    2. 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:

    <img width="1134" alt="schermafbeelding 2017-09-02 om 23 42 29" src="">
    1. 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++.

Tree-SHA512: e364f19b4ac19f910f6e8d6533357f57ccddcbd9d53dcfaf923d424d2b9711446d6f36da193208b35788ca21863eadaa7becd9ad890334d334bccf8c2e63dee1

Backport of Core PR11220

Note: the exceptions we check for are slightly different and as follows:

Depends on D4081

Test Plan
make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Sep 11 2019, 17:36
Owners added a reviewer: Restricted Owners Package.Sep 11 2019, 17:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 11 2019, 17:36
nakihito planned changes to this revision.Sep 11 2019, 17:36
nakihito requested review of this revision.Sep 11 2019, 17:50
nakihito added inline comments.
457 ↗(On Diff #11209)

This differs from Core's exception because of D1739.

531 ↗(On Diff #11209)

This differs from Core's error message because of D202

jasonbcox requested changes to this revision.Sep 11 2019, 17:57
jasonbcox added inline comments.
528 ↗(On Diff #11209)

Fix comment

This revision now requires changes to proceed.Sep 11 2019, 17:57
nakihito updated this revision to Diff 11212.Sep 11 2019, 18:10

Fixed comment.

jasonbcox accepted this revision.Sep 11 2019, 20:31
This revision is now accepted and ready to land.Sep 11 2019, 20:31
nakihito updated this revision to Diff 13018.EditedSep 19 2019, 19:58

Rebased off D4081.

nakihito edited the summary of this revision. (Show Details)Sep 19 2019, 19:58