Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rABC54155de0504f: [backport#16194] refactoring: remove mapBlockIndex global

Summary

in lieu of ::BlockIndex().

https://github.com/bitcoin/bitcoin/pull/16194/commits/682a1d0f2004d808b87b3106d0dfae547005e638

Depends on D6968

Concludes backport of Core PR16194

Notes: This backport originally changes LookupBlockIndex from being defined inline in validation.h to declared there and defined in validation.cpp.
Since we have that function defined in chain.h, I have chosen to defer to the change in the backport to stick closer to Core.
That has the unfortunate side effect of bringing back the checkpoints -> validation -> checkpoints circular dependency, that I plan on resolving by backporting Core PR15655, if this diff is landed as is.

Test Plan

ninja check-all --extended

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

majcosta created this revision.Jul 17 2020, 22:34

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 17 2020, 22:34

majcosta requested review of this revision.Jul 17 2020, 22:34

Harbormaster completed remote builds in B12055: Diff 22349.Jul 17 2020, 22:46

majcosta edited the summary of this revision. (Show Details)Jul 17 2020, 22:47

majcosta edited the summary of this revision. (Show Details)

deadalnix requested changes to this revision.Jul 19 2020, 20:14

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/chain.h
261 ↗	(On Diff #22349)	Can you keep this in chain.h/cpp ? This is better than putting everything in validation.h/cpp .
src/rpc/blockchain.cpp
1777 ↗	(On Diff #22349)	Use the same pattern as in invalidateblock . Also, this is worth doing on its own, so it doesn't get mixed with the PR.

This revision now requires changes to proceed.Jul 19 2020, 20:14

rebase on top of D6984

majcosta planned changes to this revision.Jul 21 2020, 01:08

Harbormaster completed remote builds in B12076: Diff 22387.Jul 21 2020, 01:22

majcosta requested review of this revision.Jul 21 2020, 19:31

majcosta marked an inline comment as done.

majcosta added inline comments.

src/chain.h
261 ↗	(On Diff #22349)	I don't think this is possible without some major refactoring, unless I'm missing something LookupBlockIndex now needs BlockManager instead of just BlockMap, which is defined in validation.h, so chain.h would have to include validation.h, which would have to include chain.h, ugh

majcosta marked an inline comment as done.Jul 21 2020, 19:31

majcosta added inline comments.

src/chain.h
261 ↗	(On Diff #22349)	BlockManager is in validation.h, not BlockMap*

majcosta added a child revision: D6994: [backport#15655] Resolve the checkpoints <-> validation circular dependency.Jul 21 2020, 20:36

updated lint-circular-dependencies.sh

Harbormaster completed remote builds in B12092: Diff 22410.Jul 21 2020, 21:08

Can't blockmanager be moved in its own header or something? Including validation.h everywhere like this is definitively not desirable, and solving circular dependencies by putting everything in a single blob is *NOT* a solution.

deadalnix requested changes to this revision.Jul 22 2020, 14:21

This revision now requires changes to proceed.Jul 22 2020, 14:21

deadalnix added inline comments.Sep 22 2020, 21:43

src/avalanche/test/processor_tests.cpp
12 ↗	(On Diff #22410)	Comment that this is for the function that were in chain.h and that this can be removed once they are removed from validation.h
src/checkpoints.cpp
9 ↗	(On Diff #22410)	dito

rebase and add annotation that additional includes of validation.h are to be removed as soon as LookupBlockIndex is refactored

deadalnix accepted this revision.Sep 22 2020, 23:05

This revision is now accepted and ready to land.Sep 22 2020, 23:05

Harbormaster completed remote builds in B12873: Diff 23703.Sep 22 2020, 23:18

Closed by commit rABC54155de0504f: [backport#16194] refactoring: remove mapBlockIndex global (authored by James O'Beirne <james.obeirne@gmail.com>, committed by majcosta). · Explain WhySep 22 2020, 23:56

This revision was automatically updated to reflect the committed changes.

majcosta added a commit: rABC54155de0504f: [backport#16194] refactoring: remove mapBlockIndex global.

[backport#16194] refactoring: remove mapBlockIndex global
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 23710

src/avalanche/test/processor_tests.cpp

src/chain.h

src/checkpoints.cpp

src/init.cpp

src/rpc/blockchain.cpp

src/txdb.cpp

src/validation.h

src/validation.cpp

src/wallet/test/wallet_tests.cpp

test/lint/lint-circular-dependencies.sh

[backport#16194] refactoring: remove mapBlockIndex globalClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 23710

src/avalanche/test/processor_tests.cpp

src/chain.h

src/checkpoints.cpp

src/init.cpp

src/rpc/blockchain.cpp

src/txdb.cpp

src/validation.h

src/validation.cpp

src/wallet/test/wallet_tests.cpp

test/lint/lint-circular-dependencies.sh

[backport#16194] refactoring: remove mapBlockIndex global
ClosedPublic
Actions

Revision Contents
Changeset List