Deduplicate CMerkleBlock construction code, add test coverage #11293

jamesob · 2017-09-09T17:00:15Z

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

After

practicalswift · 2017-09-11T09:29:45Z

Concept ACK. Very nice! :-)

promag

Separate refactor and test in 2 commits?

promag · 2017-09-11T09:26:26Z

src/merkleblock.h

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.

jamesob · 2017-09-12T00:46:26Z

@promag I've split this into two commits as requested. Thanks for the look.

sipa · 2017-09-12T02:12:32Z

src/merkleblock.cpp

You can avoid constructing an intermediate std::pair<unsigned int, uint256>, by using:

vMatchedTxn.emplace_back(i, hash);

sipa · 2017-09-12T02:13:26Z

src/merkleblock.cpp

Use txids->count(hash) instead.

sipa · 2017-09-12T02:14:16Z

src/merkleblock.cpp

Use filter->IsRelevantAndUpdate(*block.vtx[i]).

jamesob · 2017-09-12T02:58:34Z

Changed per feedback; thanks @sipa.

promag

utACK, some nits though.

promag · 2017-09-12T21:48:35Z

src/merkleblock.h

Should have tested my suggestion. Agree with this approach.

promag · 2017-09-12T21:49:26Z

src/test/bloom_tests.cpp

Nit, move before #include "random.h".

promag · 2017-09-12T21:51:15Z

src/merkleblock.cpp

Nit, just

if (txids && txids->count(hash)) {

Same below.

jamesob · 2017-09-13T15:40:35Z

Incorporated feedback; thanks @promag.

jamesob · 2017-09-15T19:58:39Z

@promag @sipa anything else you'd like to see done here?

sipa · 2017-09-16T00:26:33Z

utACK 17dd0973a63706717905bb3865129a32a4287adf

TheBlueMatt

Looks good, would be nice to test a few more things since the goal is to add test coverage.

TheBlueMatt · 2017-09-19T19:37:31Z

src/test/merkleblock_tests.cpp

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.

TheBlueMatt · 2017-09-19T21:00:51Z

src/test/merkleblock_tests.cpp

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.

TheBlueMatt · 2017-09-19T21:02:13Z

src/test/merkleblock_tests.cpp

Would be neat to also test the vIndex values here, too, no?

Good idea! Added.

@sipa

Incorporates feedback suggested by @sipa, @promag, @TheBlueMatt.

jamesob · 2017-09-21T04:15:19Z

@TheBlueMatt thanks for the look. I've updated the tests to be a little more comprehensive per your feedback.

TheBlueMatt · 2017-09-21T20:10:35Z

utACK 46ce223

sipa · 2017-09-21T20:20:16Z

utACK 46ce223

jamesob · 2017-09-23T16:37:16Z

Anything else needed here @laanwj?

gmaxwell

ACK.

jnewbery

Tested ACK 46ce223

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;

maflcko · 2017-10-03T11:19:55Z

utACK 46ce223

promag · 2017-10-03T11:58:53Z

utACK 46ce223.

…verage 46ce223 Add tests for CMerkleBlock usage with txids specified (James O'Beirne) 5ab586f 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

…verage Summary: 46ce223 Add tests for CMerkleBlock usage with txids specified (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 Changes made in D3371 conflict with the original PR; the constructor has been updated to the form it would take if D3371 would have been implemented after this refactoring. This private constructor is more strict than the original one (the original could be called with either, both, or neither of the `filter` and `txids` parameters). Partial backport of Core PR11293 bitcoin/bitcoin#11293 Also includes left over changes to PR12920 bitcoin/bitcoin#12920 Test Plan: make check ninja check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, nakihito Reviewed By: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, nakihito Differential Revision: https://reviews.bitcoinabc.org/D3689

…test coverage 46ce223 Add tests for CMerkleBlock usage with txids specified (James O'Beirne) 5ab586f 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

jamesob force-pushed the dedup-cmerkleblock branch from 700a113 to 5bd892c Compare September 9, 2017 17:44

fanquake added the Refactoring label Sep 10, 2017

promag reviewed Sep 11, 2017

View reviewed changes

jamesob force-pushed the dedup-cmerkleblock branch from 5bd892c to 75d8f46 Compare September 12, 2017 00:45

sipa reviewed Sep 12, 2017

View reviewed changes

jamesob force-pushed the dedup-cmerkleblock branch from 75d8f46 to 1f6fe13 Compare September 12, 2017 02:32

promag reviewed Sep 12, 2017

View reviewed changes

jamesob force-pushed the dedup-cmerkleblock branch from 1f6fe13 to 17dd097 Compare September 13, 2017 15:40

TheBlueMatt reviewed Sep 19, 2017

View reviewed changes

jamesob added 2 commits September 20, 2017 20:35

Consolidate CMerkleBlock constructor into a single method

5ab586f

Incorporates feedback suggested by @sipa, @promag, @TheBlueMatt.

Add tests for CMerkleBlock usage with txids specified

46ce223

jamesob force-pushed the dedup-cmerkleblock branch from 17dd097 to 46ce223 Compare September 21, 2017 03:36

gmaxwell approved these changes Sep 25, 2017

View reviewed changes

jnewbery reviewed Oct 2, 2017

View reviewed changes

maflcko added the Tests label Oct 3, 2017

maflcko merged commit 46ce223 into bitcoin:master Oct 3, 2017

dagurval mentioned this pull request Jun 13, 2018

Merkleblock tests and refactor bitcoinxt/bitcoinxt#454

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Deduplicate CMerkleBlock construction code, add test coverage #11293

Deduplicate CMerkleBlock construction code, add test coverage #11293

Uh oh!

Conversation

jamesob commented Sep 9, 2017

Before

After

Uh oh!

practicalswift commented Sep 11, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 12, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 13, 2017

Uh oh!

jamesob commented Sep 15, 2017

Uh oh!

sipa commented Sep 16, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Sep 21, 2017

Uh oh!

TheBlueMatt commented Sep 21, 2017

Uh oh!

sipa commented Sep 21, 2017

Uh oh!

jamesob commented Sep 23, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 3, 2017

Uh oh!

promag commented Oct 3, 2017

Uh oh!

Reviewers