Page MenuHomePhabricator
Differential D3689

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

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

Details

Reviewers
deadalnix
Fabien
jasonbcox
nakihito
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGINGeb8ed1fc6683: Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage
rABCeb8ed1fc6683: Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage
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 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
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 Passed
Unit
No Test Coverage
Build Status
Buildable 7714
Build 13467: Bitcoin ABC Buildbot (legacy)
Build 13466: 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
teamcity added a comment.Jul 17 2019, 20:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 17 2019, 21:00

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6881: Diff 10338.Jul 17 2019, 21:00
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.

teamcity added a comment.Jul 18 2019, 18:05

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 18 2019, 18:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6886: Diff 10349.Jul 18 2019, 18:11
nakihito edited the test plan for this revision. (Show Details)Jul 19 2019, 01:06
Fabien accepted this revision.Jul 23 2019, 10:20
nakihito added a child revision: D3882: Merge #12920: test: Fix sign for expected values.Aug 15 2019, 18:57
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 mentioned this in D3882: Merge #12920: test: Fix sign for expected values.Sep 18 2019, 17:27
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
teamcity added a comment.Oct 8 2019, 00:04

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito requested review of this revision.Oct 8 2019, 00:06
nakihito edited the summary of this revision. (Show Details)
teamcity added a comment.Oct 8 2019, 00:17

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7714: Diff 13399.Oct 8 2019, 00:17
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

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
teamcity added a comment.Oct 9 2019, 21:09

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7728: Diff 13477.Oct 9 2019, 21:23
teamcity added a comment.Oct 9 2019, 21:23

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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
teamcity added a comment.Oct 14 2019, 18:07

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 14 2019, 18:23

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7760: Diff 13531.Oct 14 2019, 18:23
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)Oct 16 2019, 02:08
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
nakihito planned changes to this revision.Nov 26 2019, 19:38

Will work with @markblundeberg to make this better.

markblundeberg commandeered this revision.Dec 2 2019, 23:39
markblundeberg edited reviewers, added: nakihito; removed: markblundeberg.
markblundeberg updated this revision to Diff 14582.Dec 2 2019, 23:43

update ctor

teamcity added a comment.Dec 2 2019, 23:43

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg edited the summary of this revision. (Show Details)Dec 2 2019, 23:46
teamcity added a comment.Dec 2 2019, 23:59

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8379: Diff 14582.Dec 2 2019, 23:59
jasonbcox accepted this revision.Dec 3 2019, 00:22

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...

nakihito accepted this revision.Dec 3 2019, 00:41
deadalnix accepted this revision.Dec 3 2019, 17:50
deadalnix added inline comments.
src/merkleblock.cpp
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
markblundeberg updated this revision to Diff 14600.Dec 3 2019, 22:16

rm assert & update comment

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

teamcity added a comment.Dec 3 2019, 22:16

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 3 2019, 22:31

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8392: Diff 14600.Dec 3 2019, 22:31
Closed by commit rABCeb8ed1fc6683: Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage (authored by MarcoFalke <falke.marco@gmail.com>, committed by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyDec 3 2019, 22:54
This revision was automatically updated to reflect the committed changes.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rABCeb8ed1fc6683: Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rSTAGINGeb8ed1fc6683: Merge #11293: Deduplicate CMerkleBlock construction code, add test coverage.Apr 7 2020, 21:58

Revision Contents
Changeset List

Diff 13399

View Options

src/Makefile.test.include

Loading...
View Options

src/test/CMakeLists.txt

Loading...
View Options

src/test/bloom_tests.cpp

Loading...
View Options

src/test/merkleblock_tests.cpp

Loading...
View Options

src/test/test_bitcoin.h

Loading...
View Options

src/test/test_bitcoin.cpp

Loading...