HomePhabricator

Merge #11220: Check specific validation error in miner tests

Description

Merge #11220: Check specific validation error in miner tests

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

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

Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D4042

Details

Provenance
Wladimir J. van der Laan <laanwj@gmail.com>Authored on Dec 19 2017, 12:04
nakihitoCommitted on Sep 24 2019, 23:46
nakihitoPushed on Sep 24 2019, 23:46
Reviewer
Restricted Owners Package
Differential Revision
D4042: Merge #11220: Check specific validation error in miner tests
Parents
rABC46b2cbc8507b: Remove redundant variables, statements and forward declarations
Branches
Unknown
Tags
Unknown