From 3e1ee310437f4c93113f6121425beffdc94702c2 Mon Sep 17 00:00:00 2001 From: sean Date: Tue, 21 Nov 2017 15:40:02 -0800 Subject: [PATCH] [Tests] Adding unit tests for GetDifficulty in blockchain.cpp. blockchain.cpp has low unit test coverage. This commit is intended to start improving its code coverage to reasonable levels. One or more follow up commits will complete the task that this commit is starting (though the usefulness of this commit is not dependent upon later commits). Note that these tests were not written based upon a specification of how GetDifficulty *should* work, but rather how it actually *does* work. As a result, if there are any bugs in the current GetDifficulty implementation, these unit tests serve to lock them in rather than expose them. -- Why has blockchain.cpp been modified if this is a unit testing change? Since the existing GetDifficulty function relies on a global variable, chainActive, it was not suitable for unit testing purposes. Both the existing GetDifficulty function and the unit tests now call through to a new, more modular version of GetDifficulty that can work on any chain, not just chainActive. -- Why does blockchain_tests.cpp directly include blockchain.cpp instead of blockchain.h? While the new GetDifficulty function's signature is arguably better than the old one's, it still isn't great, and doesn't seem to warrant inclusion as part of the blockchain.h API, especially since only test code is directly using it. If a better way of exposing the new GetDifficulty function to unit tests exists, please mention it and the commit will be updated accordingly. -- Why is the test fixture named blockchain_difficulty_tests rather than blockchain_tests? The Bitcoin Core policy for naming unit test files is to match the the file under test ("blockchain" becomes "blockchain_tests"). While this commit complies with that, blockchain.cpp is a massive file, such that having all of the unit tests in one file will tend towards disorder. Since there will be a lot more tests added to this file, the intention is to divide up different types of tests into different test fixtures within the same file. --- src/Makefile.test.include | 1 + src/rpc/blockchain.cpp | 15 ++-- src/test/blockchain_tests.cpp | 126 ++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 src/test/blockchain_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d3e7b5da1..fb94821b0 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -33,6 +33,7 @@ BITCOIN_TESTS =\ test/base64_tests.cpp \ test/bech32_tests.cpp \ test/bip32_tests.cpp \ + test/blockchain_tests.cpp \ test/blockencodings_tests.cpp \ test/bloom_tests.cpp \ test/bswap_tests.cpp \ diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8fff937d8..9c5ea7238 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -47,18 +47,20 @@ static CUpdatedBlock latestblock; extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry); -double GetDifficulty(const CBlockIndex* blockindex) +/* Calculate the difficulty for a given block index, + * or the block index of the given chain. + */ +double GetDifficulty(const CChain& chain, const CBlockIndex* blockindex) { if (blockindex == nullptr) { - if (chainActive.Tip() == nullptr) + if (chain.Tip() == nullptr) return 1.0; else - blockindex = chainActive.Tip(); + blockindex = chain.Tip(); } int nShift = (blockindex->nBits >> 24) & 0xff; - double dDiff = (double)0x0000ffff / (double)(blockindex->nBits & 0x00ffffff); @@ -76,6 +78,11 @@ double GetDifficulty(const CBlockIndex* blockindex) return dDiff; } +double GetDifficulty(const CBlockIndex* blockindex) +{ + return GetDifficulty(chainActive, blockindex); +} + UniValue blockheaderToJSON(const CBlockIndex* blockindex) { AssertLockHeld(cs_main); diff --git a/src/test/blockchain_tests.cpp b/src/test/blockchain_tests.cpp new file mode 100644 index 000000000..55fdd2c07 --- /dev/null +++ b/src/test/blockchain_tests.cpp @@ -0,0 +1,126 @@ +#include + +#include "stdlib.h" + +#include "rpc/blockchain.cpp" +#include "test/test_bitcoin.h" + +/* Equality between doubles is imprecise. Comparison should be done + * with a small threshold of tolerance, rather than exact equality. + */ +bool DoubleEquals(double a, double b, double epsilon) +{ + return std::abs(a - b) < epsilon; +} + +CBlockIndex* CreateBlockIndexWithNbits(uint32_t nbits) +{ + CBlockIndex* block_index = new CBlockIndex(); + block_index->nHeight = 46367; + block_index->nTime = 1269211443; + block_index->nBits = nbits; + return block_index; +} + +CChain CreateChainWithNbits(uint32_t nbits) +{ + CBlockIndex* block_index = CreateBlockIndexWithNbits(nbits); + CChain chain; + chain.SetTip(block_index); + return chain; +} + +void RejectDifficultyMismatch(double difficulty, double expected_difficulty) { + BOOST_CHECK_MESSAGE( + DoubleEquals(difficulty, expected_difficulty, 0.00001), + "Difficulty was " + std::to_string(difficulty) + + " but was expected to be " + std::to_string(expected_difficulty)); +} + +/* Given a BlockIndex with the provided nbits, + * verify that the expected difficulty results. + */ +void TestDifficulty(uint32_t nbits, double expected_difficulty) +{ + CBlockIndex* block_index = CreateBlockIndexWithNbits(nbits); + /* Since we are passing in block index explicitly, + * there is no need to set up anything within the chain itself. + */ + CChain chain; + + double difficulty = GetDifficulty(chain, block_index); + delete block_index; + + RejectDifficultyMismatch(difficulty, expected_difficulty); +} + +BOOST_FIXTURE_TEST_SUITE(blockchain_difficulty_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(get_difficulty_for_very_low_target) +{ + TestDifficulty(0x1f111111, 0.000001); +} + +BOOST_AUTO_TEST_CASE(get_difficulty_for_low_target) +{ + TestDifficulty(0x1ef88f6f, 0.000016); +} + +BOOST_AUTO_TEST_CASE(get_difficulty_for_mid_target) +{ + TestDifficulty(0x1df88f6f, 0.004023); +} + +BOOST_AUTO_TEST_CASE(get_difficulty_for_high_target) +{ + TestDifficulty(0x1cf88f6f, 1.029916); +} + +BOOST_AUTO_TEST_CASE(get_difficulty_for_very_high_target) +{ + TestDifficulty(0x12345678, 5913134931067755359633408.0); +} + +// Verify that difficulty is 1.0 for an empty chain. +BOOST_AUTO_TEST_CASE(get_difficulty_for_null_tip) +{ + CChain chain; + double difficulty = GetDifficulty(chain, nullptr); + RejectDifficultyMismatch(difficulty, 1.0); +} + +/* Verify that if difficulty is based upon the block index + * in the chain, if no block index is explicitly specified. + */ +BOOST_AUTO_TEST_CASE(get_difficulty_for_null_block_index) +{ + CChain chain = CreateChainWithNbits(0x1df88f6f); + + double difficulty = GetDifficulty(chain, nullptr); + delete chain.Tip(); + + double expected_difficulty = 0.004023; + + RejectDifficultyMismatch(difficulty, expected_difficulty); +} + +/* Verify that difficulty is based upon the explicitly specified + * block index rather than being taken from the provided chain, + * when both are present. + */ +BOOST_AUTO_TEST_CASE(get_difficulty_for_block_index_overrides_tip) +{ + CChain chain = CreateChainWithNbits(0x1df88f6f); + /* This block index's nbits should be used + * instead of the chain's when calculating difficulty. + */ + CBlockIndex* override_block_index = CreateBlockIndexWithNbits(0x12345678); + + double difficulty = GetDifficulty(chain, override_block_index); + delete chain.Tip(); + delete override_block_index; + + RejectDifficultyMismatch(difficulty, 5913134931067755359633408.0); +} + +BOOST_AUTO_TEST_SUITE_END()