Skip to content

Commit

Permalink
Merge #11220: Check specific validation error in miner tests
Browse files Browse the repository at this point in the history
Summary:
12781db [Tests] check specific validation error in miner tests (Sjors Provoost)

Pull request description:

  ## 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](http://boost.2283326.n4.nabble.com/Test-BOOST-CHECK-EXCEPTION-error-message-still-vague-tt4683257.html#a4683554), 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">

  ## 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](https://github.com/bitcoin/bitcoin/blob/v0.15.0rc3/src/test/serialize_tests.cpp#L245) 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
bitcoin/bitcoin#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
  • Loading branch information
laanwj authored and Nico Guiton committed Sep 24, 2019
1 parent 46b2cbc commit f7bae40
Showing 1 changed file with 24 additions and 10 deletions.
34 changes: 24 additions & 10 deletions src/test/miner_tests.cpp
Expand Up @@ -30,6 +30,18 @@

BOOST_FIXTURE_TEST_SUITE(miner_tests, TestingSetup)

// BOOST_CHECK_EXCEPTION predicates to check the specific validation error
class HasReason {
public:
HasReason(const std::string &reason) : m_reason(reason) {}
bool operator()(const std::runtime_error &e) const {
return std::string(e.what()).find(m_reason) != std::string::npos;
};

private:
const std::string m_reason;
};

static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE_PER_KB);

static BlockAssembler AssemblerForTest(const CChainParams &params,
Expand Down Expand Up @@ -343,9 +355,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) {
tx.vin[0].prevout = COutPoint(txid, 0);
}

BOOST_CHECK_THROW(
BOOST_CHECK_EXCEPTION(
AssemblerForTest(chainparams, g_mempool).CreateNewBlock(scriptPubKey),
std::runtime_error);
std::runtime_error, HasReason("bad-blk-sigops"));
g_mempool.clear();

tx.vin[0].prevout = COutPoint(txFirst[0]->GetId(), 0);
Expand Down Expand Up @@ -399,9 +411,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) {
// Orphan in mempool, template creation fails.
TxId txid = tx.GetId();
g_mempool.addUnchecked(txid, entry.Fee(LOWFEE).Time(GetTime()).FromTx(tx));
BOOST_CHECK_THROW(
BOOST_CHECK_EXCEPTION(
AssemblerForTest(chainparams, g_mempool).CreateNewBlock(scriptPubKey),
std::runtime_error);
std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
g_mempool.clear();

// Child with higher priority than parent.
Expand Down Expand Up @@ -436,9 +448,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) {
g_mempool.addUnchecked(
txid,
entry.Fee(LOWFEE).Time(GetTime()).SpendsCoinbase(false).FromTx(tx));
BOOST_CHECK_THROW(
// Should throw bad-tx-coinbase
BOOST_CHECK_EXCEPTION(
AssemblerForTest(chainparams, g_mempool).CreateNewBlock(scriptPubKey),
std::runtime_error);
std::runtime_error, HasReason("bad-tx-coinbase"));
g_mempool.clear();

// Double spend txn pair in mempool, template creation fails.
Expand All @@ -455,9 +468,9 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) {
g_mempool.addUnchecked(
txid,
entry.Fee(HIGHFEE).Time(GetTime()).SpendsCoinbase(true).FromTx(tx));
BOOST_CHECK_THROW(
BOOST_CHECK_EXCEPTION(
AssemblerForTest(chainparams, g_mempool).CreateNewBlock(scriptPubKey),
std::runtime_error);
std::runtime_error, HasReason("bad-txns-inputs-missingorspent"));
g_mempool.clear();

// Subsidy changing.
Expand Down Expand Up @@ -508,9 +521,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) {
g_mempool.addUnchecked(
txid,
entry.Fee(LOWFEE).Time(GetTime()).SpendsCoinbase(false).FromTx(tx));
BOOST_CHECK_THROW(
// Should throw blk-bad-inputs
BOOST_CHECK_EXCEPTION(
AssemblerForTest(chainparams, g_mempool).CreateNewBlock(scriptPubKey),
std::runtime_error);
std::runtime_error, HasReason("blk-bad-inputs"));
g_mempool.clear();

// Delete the dummy blocks again.
Expand Down

0 comments on commit f7bae40

Please sign in to comment.