Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

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

![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 have decided the issue of code ownership. In addition, the changes to the constructor make the code both more difficult to read and harder to maintain without providing any real benefit. Because of these factors, I skipped the changes to the constructor and only include the changes/additions to tests.

Partial backport of Core PR11293 (only including test changes)
https://github.com/bitcoin/bitcoin/pull/11293/

Also includes left over changes to PR12920
https://github.com/bitcoin/bitcoin/pull/12920/

Test Plan
make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11293
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7760
Build 13559: Bitcoin ABC Buildbot
Build 13558: arc lint + arc unit

Event Timeline

nakihito created this revision.Jul 17 2019, 20:51
Owners added a reviewer: Restricted Owners Package.Jul 17 2019, 20:51
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 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
nakihito updated this revision to Diff 10349.Jul 18 2019, 18:05

Added CMake.

nakihito edited the test plan for this revision. (Show Details)Jul 19 2019, 01:06
Fabien accepted this revision.Jul 23 2019, 10:20
deadalnix added inline comments.Aug 28 2019, 17:41
src/merkleblock.cpp
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
nakihito planned changes to this revision.Sep 11 2019, 20:12
nakihito updated this revision to Diff 13399.Oct 8 2019, 00:04

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 planned changes to this revision.Oct 8 2019, 00:04
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.
src/merkleblock.cpp
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.

src/merkleblock.h
170 ↗(On Diff #10349)

Restore.

179 ↗(On Diff #10349)

Why was this removed?

185 ↗(On Diff #10349)

dito

200 ↗(On Diff #10349)

dito

src/test/merkleblock_tests.cpp
23 ↗(On Diff #13399)

Quite evidently these aren't txhashes, but txids.

This revision now requires changes to proceed.Oct 8 2019, 16:12
nakihito updated this revision to Diff 13477.Oct 9 2019, 21:09

txhashx -> txidx and rebase.

nakihito planned changes to this revision.Oct 9 2019, 21:09
nakihito edited the summary of this revision. (Show Details)Oct 14 2019, 17:59

Updated summary to reflect changes.

src/merkleblock.h
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.

nakihito updated this revision to Diff 13531.Oct 14 2019, 18:07

Restored comment change.

nakihito planned changes to this revision.Oct 14 2019, 18:07
nakihito requested review of this revision.Oct 14 2019, 22:34

Updated summary to reflect discuss on why changes were reverted.

nakihito edited the summary of this revision. (Show Details)Wed, Oct 16, 02:08
deadalnix requested changes to this revision.Thu, Oct 17, 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.Thu, Oct 17, 01:44