Page MenuHomePhabricator

Merge #11220: Check specific validation error in miner tests
ClosedPublic

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

Details

Summary

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="https://user-images.githubusercontent.com/10217/29998976-815cabce-9038-11e7-9c46-f5f6cfb0ca7d.png">
    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
https://github.com/bitcoin/bitcoin/pull/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

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11220
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7395
Build 12833: Bitcoin ABC Buildbot (legacy)
Build 12832: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Sep 11 2019, 17:36
nakihito added inline comments.
src/test/miner_tests.cpp
457

This differs from Core's exception because of D1739.

531

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.
src/test/miner_tests.cpp
528

Fix comment

This revision now requires changes to proceed.Sep 11 2019, 17:57
This revision is now accepted and ready to land.Sep 11 2019, 20:31