Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 7516
Build 13072: Bitcoin ABC Teamcity Staging
Build 13071: arc lint + arc unit

Event Timeline

nakihito created this revision.Wed, Sep 11, 17:36
Owners added a reviewer: Restricted Owners Package.Wed, Sep 11, 17:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Sep 11, 17:36
nakihito planned changes to this revision.Wed, Sep 11, 17:36
nakihito requested review of this revision.Wed, Sep 11, 17:50
nakihito added inline comments.
src/test/miner_tests.cpp
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.Wed, Sep 11, 17:57
jasonbcox added inline comments.
src/test/miner_tests.cpp
528 ↗(On Diff #11209)

Fix comment

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

Fixed comment.

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

Rebased off D4018.

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