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.
152 | @@ -154,6 +153,10 @@ class CMerkleBlock 153 | READWRITE(header); 154 | READWRITE(txn); 155 | } 156 | + 157 | +private: 158 | + // Combined constructor to consolidate code 159 | + CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std::set<uint256>* txids);
Instead of pointers, you can make it receive references, just pass either CBloomFilter() or std::set<uint256>(), and remove the != nullptr conditions above. Note that CBloomFilter is optimized when it is empty.
If you do this then this constructor can be public.
I don't think this will work in the case of filter being unused. I'm getting the following compilation error:
In file included from merkleblock.cpp:6:0:
merkleblock.h: In constructor ‘CMerkleBlock::CMerkleBlock(const CBlock&, const std::set<uint256>&)’:
merkleblock.h:145:93: error: invalid initialization of non-const reference of type ‘CBloomFilter&’ from an rvalue of type ‘CBloomFilter’
CMerkleBlock(const CBlock& block, const std::set<uint256>& txids) : CMerkleBlock(block, CBloomFilter(), txids) {}
^~~~~~~~~~~~~~
merkleblock.h:148:5: note: initializing argument 2 of ‘CMerkleBlock::CMerkleBlock(const CBlock&, CBloomFilter&, const std::set<uint256>&)’
CMerkleBlock(const CBlock& block, CBloomFilter& filter, const std::set<uint256>& txids);
^~~~~~~~~~~~
Apparently we can't create an unnamed, non-const CBloomFilter reference from an rvalue. I'm not sure how to avoid this when using constructor delegation.
You're not allowed to pass a temporary into a method that expects a non-const reference. Non-const references are intended for output arguments, and passing a temporary would mean there is effectively no output argument the caller can observe.
It seems to be that using a pointer which can be optionally nullptr is the correct approach here.
Should have tested my suggestion. Agree with this approach.
Concept ACK. Very nice! :-)
Separate refactor and test in 2 commits?
27 | + if (txids != nullptr && (*txids).count(hash)) { 28 | vMatch.push_back(true); 29 | - else 30 | + } else if (filter != nullptr && (*filter).IsRelevantAndUpdate(*block.vtx[i])) { 31 | + vMatch.push_back(true); 32 | + vMatchedTxn.push_back(std::make_pair(i, hash));
You can avoid constructing an intermediate std::pair<unsigned int, uint256>, by using:
vMatchedTxn.emplace_back(i, hash);
22 | @@ -48,10 +23,14 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, const std::set<uint256>& txids) 23 | for (unsigned int i = 0; i < block.vtx.size(); i++) 24 | { 25 | const uint256& hash = block.vtx[i]->GetHash(); 26 | - if (txids.count(hash)) 27 | + if (txids != nullptr && (*txids).count(hash)) {
Use txids->count(hash) instead.
22 | @@ -48,10 +23,14 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, const std::set<uint256>& txids) 23 | for (unsigned int i = 0; i < block.vtx.size(); i++) 24 | { 25 | const uint256& hash = block.vtx[i]->GetHash(); 26 | - if (txids.count(hash)) 27 | + if (txids != nullptr && (*txids).count(hash)) { 28 | vMatch.push_back(true); 29 | - else 30 | + } else if (filter != nullptr && (*filter).IsRelevantAndUpdate(*block.vtx[i])) {
Use filter->IsRelevantAndUpdate(*block.vtx[i]).
13 | @@ -14,6 +14,7 @@ 14 | #include "uint256.h" 15 | #include "util.h" 16 | #include "utilstrencodings.h" 17 | +#include "primitives/block.h"
Nit, move before #include "random.h".
22 | @@ -48,10 +23,14 @@ CMerkleBlock::CMerkleBlock(const CBlock& block, const std::set<uint256>& txids) 23 | for (unsigned int i = 0; i < block.vtx.size(); i++) 24 | { 25 | const uint256& hash = block.vtx[i]->GetHash(); 26 | - if (txids.count(hash)) 27 | + if (txids != nullptr && txids->count(hash)) {
Nit, just
if (txids && txids->count(hash)) {
Same below.
utACK, some nits though.
utACK 17dd0973a63706717905bb3865129a32a4287adf
0 | @@ -0,0 +1,61 @@ 1 | +// Copyright (c) 2012-2017 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | +
You've added a bunch of trailing whitespace in this file (as well as test_bitcoin.cpp in 2 places and test_bitcoin.h in one). If you re-push this PR should fail travis due to #11300.
Ah, totally right. Editor config adjusted -- thanks.
23 | + uint256 txhash1 = uint256S("0x74d681e0e03bafa802c8aa084379aa98d9fcd632ddc2ed9782b586ec87451f20"); 24 | + txids1.insert(txhash1); 25 | + CMerkleBlock merkleBlock(block, txids1); 26 | + 27 | + BOOST_CHECK_EQUAL(merkleBlock.header.GetHash().GetHex(), block.GetHash().GetHex()); 28 | + BOOST_CHECK_EQUAL(merkleBlock.vMatchedTxn.size(), 0);
Seems a strange thing to test, here? Maybe document on merkleblock.h what this thing even means? (ie "Set only when constructing from a bloom filter to allow testing the set of things which matched the bloom filter directly").
Comment added inline with test as well as merkleblock.h. Basically just used your copy directly.
30 | + std::vector<uint256> vMatched; 31 | + std::vector<unsigned int> vIndex; 32 | + 33 | + BOOST_CHECK_EQUAL(merkleBlock.txn.ExtractMatches(vMatched, vIndex).GetHex(), block.hashMerkleRoot.GetHex()); 34 | + BOOST_CHECK_EQUAL(vMatched.size(), 1); 35 | + BOOST_CHECK_EQUAL(vMatched[0].ToString(), txhash1.ToString());
Would be neat to also test the vIndex values here, too, no?
Good idea! Added.
Looks good, would be nice to test a few more things since the goal is to add test coverage.
Incorporates feedback suggested by @sipa, @promag, @TheBlueMatt.
@TheBlueMatt thanks for the look. I've updated the tests to be a little more comprehensive per your feedback.
utACK 46ce223d15d4111d096f6342eb6f526d2507d7d7
utACK 46ce223d15d4111d096f6342eb6f526d2507d7d7
ACK.
Tested ACK 46ce223d15d4111d096f6342eb6f526d2507d7d7
The constructor can only be called with txids or filter specified, not both. How about making that explicit in the implementation with a comment or an assert (or both)? eg:
diff --git a/src/merkleblock.cpp b/src/merkleblock.cpp
index 3f07b4d..7fd2934 100644
--- a/src/merkleblock.cpp
+++ b/src/merkleblock.cpp
@@ -12,6 +12,9 @@
CMerkleBlock::CMerkleBlock(const CBlock& block, CBloomFilter* filter, const std::set<uint256>* txids)
{
+ // The constructor can only be called with filter or txids, not both.
+ assert(!filter or !txids);
+
header = block.GetBlockHeader();
std::vector<bool> vMatch;
utACK 46ce223d15d4111d096f6342eb6f526d2507d7d7
utACK 46ce223.