Details

Reviewers

jasonbcox
deadalnix
schancel

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Continues work on T380, dependent on D1659. Removed CBlockIndex class from chain.h into blockindex.h and updated other file dependencies.

Test Plan

Make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

SeperatingBlockIndexClass

Lint

Lint Passed

Severity	Location	Code	Message
Auto-Fix	src/versionbits.cpp:1	CFMT	Code style violation

Unit

No Test Coverage

Build Status

Buildable 3331
Build 4745: Bitcoin ABC Buildbot (legacy)
Build 4744: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

Harbormaster completed remote builds in B3094: Diff 4603.Aug 20 2018, 03:36

nakihito mentioned this in D1661: Seperated CDiskBlockIndex class from chain.h into its own .h file and updated other .cpp and .h file dependencies..Aug 20 2018, 03:47

jasonbcox requested changes to this revision.Aug 20 2018, 21:50

jasonbcox added inline comments.

src/blockindex.h
206 ↗	(On Diff #4603)	Looks like some whitespace got deleted here. Not sure if that was intentional.
src/chain.h
21 ↗	(On Diff #4603)	Is this line necessary?
src/versionbits.h
9 ↗	(On Diff #4603)	Is chain.h still necessary here?

This revision now requires changes to proceed.Aug 20 2018, 21:50

Fixed white space typo and removed unnecessary chain.h include in versionbits.h.

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

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

Harbormaster completed remote builds in B3097: Diff 4609.Aug 20 2018, 23:59

Updated some unnecessary inlcudes.

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

Harbormaster failed remote builds in B3100: Diff 4612!Aug 21 2018, 00:45

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

It's not clear to me why many of the include change are required. Except that it seems like a good change.

src/blockindex.h
1 ↗	(On Diff #4612)	The Bitcoin developers
221 ↗	(On Diff #4612)	Keep
src/blockindexworkcomparator.h
8 ↗	(On Diff #4612)
src/chain.h
63 ↗	(On Diff #4612)	This is the only place where CBlockIndex is not used by pointer and/or ref, so this would deserve to be commented on and have a plan to split this one as well so everything else in there can just use a forward declaration rather than an include.
src/checkpoints.cpp
7 ↗	(On Diff #4612)	It doesn't seems like it is required.
src/init.cpp
14 ↗	(On Diff #4612)	dito
src/rest.cpp
6 ↗	(On Diff #4612)	dito
src/rpc/blockchain.cpp
10 ↗	(On Diff #4612)	Why is it required now ?
src/test/blockencodings_tests.cpp
9 ↗	(On Diff #4612)	???

This revision now requires changes to proceed.Aug 21 2018, 13:03

Will update this patch with recommended changes as soon as soon as I can.

src/blockindex.h
1 ↗	(On Diff #4612)	Fixed.
221 ↗	(On Diff #4612)	This is just a single newline between line 221 and 222, right? I will re-add this back in.
src/chain.h
63 ↗	(On Diff #4612)	Doing this causes compile errors on my end.
src/checkpoints.cpp
7 ↗	(On Diff #4612)	Ah, you're right. Will fix.
src/init.cpp
14 ↗	(On Diff #4612)	Also, fixed. Although this might come back after CDiskBlockIndex is removed from chain.h in the next patch.
src/rest.cpp
6 ↗	(On Diff #4612)	Also fixed.
src/rpc/blockchain.cpp
10 ↗	(On Diff #4612)	Opps, this was a mistake I missed. Fixed.
src/test/blockencodings_tests.cpp
9 ↗	(On Diff #4612)	This is actually necessary now because of some overly complicated include chains. The specific function call used is CheckProofOfWork().

Removed some unnecessary include statements and fixed some formating issues.

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

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

Harbormaster completed remote builds in B3167: Diff 4712.Aug 28 2018, 03:59

jasonbcox accepted this revision.Sep 4 2018, 17:58

deadalnix requested changes to this revision.Sep 4 2018, 20:28

deadalnix added inline comments.

src/txdb.h
10 ↗	(On Diff #4712)	Remove.
src/versionbits.h
8 ↗	(On Diff #4712)	Forward declare CBlockIndex and remove.

This revision now requires changes to proceed.Sep 4 2018, 20:28

nakihito added inline comments.Sep 7 2018, 20:43

src/txdb.h
10 ↗	(On Diff #4712)	Done.
src/versionbits.h
8 ↗	(On Diff #4712)	Doing this requires adding an #include in the .cpp file. If that's preferable, just let me know.

jasonbcox added inline comments.Sep 8 2018, 22:05

src/versionbits.h
8 ↗	(On Diff #4712)	Yes, it's preferable so that other code does not end up including blockindex.h as a side effect of including versionbits.h.

Removed unnecessary includes and added forward declaration as suggested.

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

Harbormaster completed remote builds in B3233: Diff 4823.Sep 9 2018, 07:10

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

deadalnix requested changes to this revision.Sep 10 2018, 14:39

deadalnix added inline comments.

src/chain.h
63 ↗	(On Diff #4823)	Please add a comment as previously requested.

This revision now requires changes to proceed.Sep 10 2018, 14:39

Added comment signaling CDiskBlockIndex will be seperated in a subsequent diff.

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

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

Harbormaster completed remote builds in B3273: Diff 4883.Sep 11 2018, 21:28

deadalnix requested changes to this revision.Sep 12 2018, 00:38

deadalnix added inline comments.

src/chain.h
63 ↗	(On Diff #4883)	That is not not what was asked.

This revision now requires changes to proceed.Sep 12 2018, 00:38

nakihito added inline comments.Sep 17 2018, 18:47

src/chain.h
63 ↗	(On Diff #4883)	Sorry, I'm not quite sure what exactly you're asking for. Could you clarify what you want here for me?

Added TODO comment for CDiskBlockIndex.

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

Fixed comment formatting.

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

Harbormaster failed remote builds in B3330: Diff 4972!Sep 19 2018, 18:30

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

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

Harbormaster failed remote builds in B3331: Diff 4973!Sep 19 2018, 18:32

Fixed missing include statement.

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

Harbormaster completed remote builds in B3385: Diff 5072.Sep 26 2018, 19:03

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

deadalnix added inline comments.Sep 28 2018, 15:21

src/chain.h
63 ↗	(On Diff #4883)	This is the only place where CBlockIndex is not used by pointer and/or ref, so this would deserve to be commented on and have a plan to split this one as well so everything else in there can just use a forward declaration rather than an include. What do you not understand in that comment ? If you do not understand something, you need to ask.

Updated to latest version of master.

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

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

Harbormaster completed remote builds in B3691: Diff 5546.Oct 26 2018, 04:43

Trying to clear off the review queue temporarily. Will get back to review this later.

This revision now requires changes to proceed.Nov 12 2018, 22:09

nakihito abandoned this revision.Feb 4 2019, 22:42

Seperated CBlockIndex class from chain.h into it blockindex.h and updated dependencies in other .h and .cpp files.
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4973

src/Makefile.am

src/blockindex.h

src/blockindexworkcomparator.h

src/chain.h

src/miner.cpp

src/pow.cpp

src/rpc/blockchain.cpp

src/rpc/mining.cpp

src/test/blockencodings_tests.cpp

src/test/blockindex_tests.cpp

src/test/pow_tests.cpp

src/test/skiplist_tests.cpp

src/test/versionbits_tests.cpp

src/txdb.h

src/txdb.cpp

src/validation.cpp

src/versionbits.h

src/versionbits.cpp

src/wallet/rpcdump.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

Seperated CBlockIndex class from chain.h into it blockindex.h and updated dependencies in other .h and .cpp files.AbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4973

src/Makefile.am

src/blockindex.h

src/blockindexworkcomparator.h

src/chain.h

src/miner.cpp

src/pow.cpp

src/rpc/blockchain.cpp

src/rpc/mining.cpp

src/test/blockencodings_tests.cpp

src/test/blockindex_tests.cpp

src/test/pow_tests.cpp

src/test/skiplist_tests.cpp

src/test/versionbits_tests.cpp

src/txdb.h

src/txdb.cpp

src/validation.cpp

src/versionbits.h

src/versionbits.cpp

src/wallet/rpcdump.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

Seperated CBlockIndex class from chain.h into it blockindex.h and updated dependencies in other .h and .cpp files.
AbandonedPublic
Actions

Revision Contents
Changeset List