Page MenuHomePhabricator

[backport#16194] refactoring: remove mapBlockIndex global
ClosedPublic

Authored by majcosta on Jul 17 2020, 22:34.

Details

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

Event Timeline

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
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 added inline comments.
src/chain.h
261 ↗(On Diff #22349)

BlockManager is in validation.h, not BlockMap*

updated lint-circular-dependencies.sh

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
src/avalanche/test/processor_tests.cpp
12

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

dito

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

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