Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage

46ce223d1 Add tests for CMerkleBlock usage with txids specified (James O'Beirne)
5ab586f90 Consolidate CMerkleBlock constructor into a single method (James O'Beirne)

Pull request description:

  What started as a simple task to add test coverage ended up giving way to a light refactoring. This consolidates the mostly-identical `CMerkleBlock` constructors into one (using C++11 constructor delegation) and adds coverage for the by-txids construction case.

  ### Before

  ![selection_006](https://user-images.githubusercontent.com/73197/30242104-0f381fe4-9545-11e7-9617-83b87fce0456.png)

  ### After

  ![selection_008](https://user-images.githubusercontent.com/73197/30242107-1425dfaa-9545-11e7-9e6b-2c3432517dd1.png)

Tree-SHA512: eed84ed3e8bfc43473077b575c8252759a857e37275e4b36ca7cc2c17a65895e5f494bfd9d4aeab09fc6e98fc6a9c641ac7ecc0ddbeefe01a9e4308e7909e529

src/Makefile.test.include

@@ -50,6 +50,7 @@ BITCOIN_TESTS =\
   test/main_tests.cpp \
   test/mempool_tests.cpp \
   test/merkle_tests.cpp \
+  test/merkleblock_tests.cpp \
   test/miner_tests.cpp \
   test/multisig_tests.cpp \
   test/net_tests.cpp \

src/merkleblock.cpp

@@ -9,7 +9,8 @@
 #include "consensus/consensus.h"
 #include "utilstrencodings.h"
 
-CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter& filter)
+
+CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std::set<uint256>* txids)
 {
     header = block.GetBlockHeader();
 
@@ -22,36 +23,14 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter& filter)
     for (unsigned int i = 0; i < block.vtx.size(); i++)
     {
         const uint256& hash = block.vtx[i]->GetHash();
-        if (filter.IsRelevantAndUpdate(*block.vtx[i]))
-        {
+        if (txids && txids->count(hash)) {
             vMatch.push_back(true);
-            vMatchedTxn.push_back(std::make_pair(i, hash));
+        } else if (filter && filter->IsRelevantAndUpdate(*block.vtx[i])) {
+            vMatch.push_back(true);
+            vMatchedTxn.emplace_back(i, hash);
+        } else {
+            vMatch.push_back(false);
         }
-        else
-            vMatch.push_back(false);
-        vHashes.push_back(hash);
-    }
-
-    txn = CPartialMerkleTree(vHashes, vMatch);
-}
-
-CMerkleBlock::CMerkleBlock(const CBlock& block, const std::set<uint256>& txids)
-{
-    header = block.GetBlockHeader();
-
-    std::vector<bool> vMatch;
-    std::vector<uint256> vHashes;
-
-    vMatch.reserve(block.vtx.size());
-    vHashes.reserve(block.vtx.size());
-
-    for (unsigned int i = 0; i < block.vtx.size(); i++)
-    {
-        const uint256& hash = block.vtx[i]->GetHash();
-        if (txids.count(hash))
-            vMatch.push_back(true);
-        else
-            vMatch.push_back(false);
         vHashes.push_back(hash);
     }
 

src/merkleblock.h

@@ -131,8 +131,12 @@ public:
     CBlockHeader header;
     CPartialMerkleTree txn;
 
-public:
-    /** Public only for unit testing and relay testing (not relayed) */
+    /**
+     * Public only for unit testing and relay testing (not relayed).
+     *
+     * Used only when a bloom filter is specified to allow
+     * testing the transactions which matched the bloom filter.
+     */
     std::vector<std::pair<unsigned int, uint256> > vMatchedTxn;
 
     /**
@@ -140,10 +144,10 @@ public:
      * Note that this will call IsRelevantAndUpdate on the filter for each transaction,
      * thus the filter will likely be modified.
      */
-    CMerkleBlock(const CBlock& block, CBloomFilter& filter);
+    CMerkleBlock(const CBlock& block, CBloomFilter& filter) : CMerkleBlock(block, &filter, nullptr) { }
 
     // Create from a CBlock, matching the txids in the set
-    CMerkleBlock(const CBlock& block, const std::set<uint256>& txids);
+    CMerkleBlock(const CBlock& block, const std::set<uint256>& txids) : CMerkleBlock(block, nullptr, &txids) { }
 
     CMerkleBlock() {}
 
@@ -154,6 +158,10 @@ public:
         READWRITE(header);
         READWRITE(txn);
     }
+
+private:
+    // Combined constructor to consolidate code
+    CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std::set<uint256>* txids);
 };
 
 #endif // BITCOIN_MERKLEBLOCK_H

src/test/merkleblock_tests.cpp

@@ -0,0 +1,78 @@
+// Copyright (c) 2012-2017 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include "merkleblock.h"
+#include "uint256.h"
+#include "test/test_bitcoin.h"
+
+#include <boost/test/unit_test.hpp>
+
+
+BOOST_FIXTURE_TEST_SUITE(merkleblock_tests, BasicTestingSetup)
+
+/**
+ * Create a CMerkleBlock using a list of txids which will be found in the
+ * given block.
+ */
+BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_found)
+{
+    CBlock block = getBlock13b8a();
+
+    std::set<uint256> txids;
+
+    // Last txn in block.
+    uint256 txhash1 = uint256S("0x74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20");
+
+    // Second txn in block.
+    uint256 txhash2 = uint256S("0xf9fc751cb7dc372406a9f8d738d5e6f8f63bab71986a39cf36ee70ee17036d07");
+
+    txids.insert(txhash1);
+    txids.insert(txhash2);
+
+    CMerkleBlock merkleBlock(block, txids);
+
+    BOOST_CHECK_EQUAL(merkleBlock.header.GetHash().GetHex(), block.GetHash().GetHex());
+
+    // vMatchedTxn is only used when bloom filter is specified.
+    BOOST_CHECK_EQUAL(merkleBlock.vMatchedTxn.size(), 0);
+
+    std::vector<uint256> vMatched;
+    std::vector<unsigned int> vIndex;
+
+    BOOST_CHECK_EQUAL(merkleBlock.txn.ExtractMatches(vMatched, vIndex).GetHex(), block.hashMerkleRoot.GetHex());
+    BOOST_CHECK_EQUAL(vMatched.size(), 2);
+
+    // Ordered by occurrence in depth-first tree traversal.
+    BOOST_CHECK_EQUAL(vMatched[0].ToString(), txhash2.ToString());
+    BOOST_CHECK_EQUAL(vIndex[0], 1);
+
+    BOOST_CHECK_EQUAL(vMatched[1].ToString(), txhash1.ToString());
+    BOOST_CHECK_EQUAL(vIndex[1], 8);
+}
+
+
+/**
+ * Create a CMerkleBlock using a list of txids which will not be found in the
+ * given block.
+ */
+BOOST_AUTO_TEST_CASE(merkleblock_construct_from_txids_not_found)
+{
+    CBlock block = getBlock13b8a();
+
+    std::set<uint256> txids2;
+    txids2.insert(uint256S("0xc0ffee00003bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20"));
+    CMerkleBlock merkleBlock(block, txids2);
+
+    BOOST_CHECK_EQUAL(merkleBlock.header.GetHash().GetHex(), block.GetHash().GetHex());
+    BOOST_CHECK_EQUAL(merkleBlock.vMatchedTxn.size(), 0);
+
+    std::vector<uint256> vMatched;
+    std::vector<unsigned int> vIndex;
+
+    BOOST_CHECK_EQUAL(merkleBlock.txn.ExtractMatches(vMatched, vIndex).GetHex(), block.hashMerkleRoot.GetHex());
+    BOOST_CHECK_EQUAL(vMatched.size(), 0);
+    BOOST_CHECK_EQUAL(vIndex.size(), 0);
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/test_bitcoin.h

@@ -99,7 +99,7 @@ struct TestMemPoolEntryHelper
     TestMemPoolEntryHelper() :
         nFee(0), nTime(0), nHeight(1),
         spendsCoinbase(false), sigOpCost(4) { }
-    
+
     CTxMemPoolEntry FromTx(const CMutableTransaction &tx);
     CTxMemPoolEntry FromTx(const CTransaction &tx);
 
@@ -110,4 +110,7 @@ struct TestMemPoolEntryHelper
     TestMemPoolEntryHelper &SpendsCoinbase(bool _flag) { spendsCoinbase = _flag; return *this; }
     TestMemPoolEntryHelper &SigOpsCost(unsigned int _sigopsCost) { sigOpCost = _sigopsCost; return *this; }
 };
+
+CBlock getBlock13b8a();
+
 #endif