From f7bae40b393f2e13c18ea2eb2a8557568cda507d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 19 Dec 2017 13:04:58 +0100 Subject: [PATCH] Merge #11220: Check specific validation error in miner tests 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: schermafbeelding 2017-09-02 om 23 42 29 ## 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 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 --- src/test/miner_tests.cpp | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index f2fe51cda6..ad44324802 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -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 ¶ms, @@ -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); @@ -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. @@ -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. @@ -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. @@ -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.