Page MenuHomePhabricator

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

Authored by markblundeberg on Jul 17 2019, 20:51.



46ce223d1 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


### After


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

Also includes left over changes to PR12920

Test Plan
make check
ninja check

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 7728
Build 13495: Bitcoin ABC Buildbot (legacy)
Build 13494: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jul 17 2019, 20:51
deadalnix requested changes to this revision.Jul 18 2019, 12:06

cmake build

This revision now requires changes to proceed.Jul 18 2019, 12:06
24 ↗(On Diff #10349)

This clearly isn't CTOR compliant.

jasonbcox requested changes to this revision.Sep 9 2019, 22:06

Clearing this off my queue. See Amaury's comment about CTOR.

This revision now requires changes to proceed.Sep 9 2019, 22:06

Removed functionally correct, but unwanted changes to merkleblock.cpp/h. The issue of code ownership was decided when D3371 was landed. Changes are now only directed to tests.

nakihito requested review of this revision.Oct 8 2019, 00:06
nakihito edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.Oct 8 2019, 16:12
deadalnix added inline comments.
24 ↗(On Diff #10349)

It turns out it was the constructor, so I was wrong about CTOR. Adding a simple test case would have proven this immediately - such test case probably already exist and pointing to it would also be suffiscient.

170 ↗(On Diff #10349)


179 ↗(On Diff #10349)

Why was this removed?

185 ↗(On Diff #10349)


200 ↗(On Diff #10349)


23 ↗(On Diff #13399)

Quite evidently these aren't txhashes, but txids.

This revision now requires changes to proceed.Oct 8 2019, 16:12

txhashx -> txidx and rebase.

Updated summary to reflect changes.

179 ↗(On Diff #10349)

I was told that the changes made in D3371 made code ownership no longer an issue and that the changes to the constructor made the code both more difficult to read and harder to maintain.

Updated summary to reflect discuss on why changes were reverted.

deadalnix requested changes to this revision.Oct 17 2019, 01:44

The changes made in D3371 do not affect the constructor, so the rationale for not backporting makes no sense whatsoever.

This revision now requires changes to proceed.Oct 17 2019, 01:44

Will work with @markblundeberg to make this better.

markblundeberg edited reviewers, added: nakihito; removed: markblundeberg.

I still do not agree that it makes any sense to consolidate the constructors. It makes the code quality worse. But hey, we don't own that code do we? Oh wait...

deadalnix added inline comments.
14 ↗(On Diff #14582)

You don't really need to assert this. Nothing down the road specifically need to relly on it.

This revision is now accepted and ready to land.Dec 3 2019, 17:50

rm assert & update comment

note the only poor behaviour is that if both txids and filter are supplied,
then txids will be ignored