After PR #13033 and PR #13243 there is a useful framework to add more indexes other than txindex, however circular dependencies detected by lint (index/txindex -> validation -> index/txindex and index/txindex -> index/base -> validation -> index/txindex) would prevent such solution to pass CI checks without adding an exception rule in check scrypt. In case one calls newly added index method in validation.cpp, another circular dependency is created because new index is supposed to inherit from index/base. My proposition is to split validation.cpp into two parts, one with its core features / functionalities and other with utils used in other translation units that require including validation. No functional change in code is expected here as it is only moved between files, and a circular dependency is removed.
refactor: Removal of circular dependency between index/txindex, validation and index/base #13942
pull mgrychow wants to merge 6 commits into bitcoin:master from mgrychow:circular-dependency_removal changing 26 files +395 −279-
mgrychow commented at 2:48 AM on August 11, 2018: none
- mgrychow force-pushed on Aug 11, 2018
- fanquake added the label Refactoring on Aug 11, 2018
-
DrahtBot commented at 5:53 AM on August 11, 2018: contributor
<!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:
- #14111 (index: Create IndexRunner class for activing indexes. by jimpo)
- #14053 (Add address-based index (attempt 4?) by marcinja)
- #14035 (Utxoscriptindex by mgrychow)
- #14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)
- #13949 (Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact)
- #13937 (Track best-possible-headers (TheBlueMatt) by Sjors)
- #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)
- #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
- #13686 (ZMQ: Small cleanups in the ZMQ code by domob1812)
- #13258 (uint256: Remove unnecessary crypto/common.h dependency by kallewoof)
- #11652 (Add missing locks: validation.cpp + related by practicalswift)
- #11599 (scripted-diff: Small locking rename by ryanofsky)
- #10823 (Allow all mempool txs to be replaced after a configurable timeout (default 6h) by greenaddress)
- #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
- DrahtBot cross-referenced this on Aug 11, 2018 from issue Track best-possible-headers (TheBlueMatt) by Sjors
- DrahtBot cross-referenced this on Aug 11, 2018 from issue validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift
- DrahtBot cross-referenced this on Aug 11, 2018 from issue Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact
- DrahtBot cross-referenced this on Aug 11, 2018 from issue Include tinyformat as a subtree by Empact
- DrahtBot cross-referenced this on Aug 11, 2018 from issue -masterdatadir for datadir bootstrapping by kallewoof
-
Empact commented at 6:52 AM on August 11, 2018: member
How about splitting the content further to fit more functional areas? E.g. there's a pretty clear division between disk-oriented functions and block-index-oriented functions, seems they are independent of the other globals.
-
Empact commented at 5:05 PM on August 11, 2018: member
You'll want to squash 86e36e0
-
Empact commented at 7:12 PM on August 12, 2018: member
In each file, you'll want to be sure each of the definitions used are included in the file directly or in the .h for that file if used there, or forward-declared. E.g.
#include <uint256.h>invalidation_block_utils.hEvery .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.` https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md
I'm concerned about the fact that the globals are still set in
validation.cpp. Seems moving those tovalidation_globals.cppwould be more consistent with that file's purpose of containing and presenting the globals. One idea is to renamevalidation_block_utils.htochainstate.hand moveg_chainstate,mapBlockIndexandchainActiveout to it. - DrahtBot cross-referenced this on Aug 13, 2018 from issue Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact
- mgrychow force-pushed on Aug 20, 2018
-
mgrychow commented at 1:59 PM on August 20, 2018: none
Rebased, conflict solved, 86e36e064b18067f8c09f0a4f4bee55c76cbacf0 squashed. @Empact : Globals from
validation_globals.hset invalidationwere moved tovalidation_globals.cppwhere possible, but please note that there are stilltxmempool->validation->txmempoolandpolicy/fees->policy/policy->validation->policy/feescircular dependencies that prevent movingmapBlockIndex,feeEstimatorandmempoolas it would create another CD. Similarly,g_chainstatecannot yet be moved to file included byvalidation, becauseCChainStateusesConnectTracethat usesMemPoolRemovalReasonfromtxmempool, which includesvalidation(circular dep again). As fixing those other CDs is beyond this pull request scope, I suggest to do it in another PR after those other lint issues are fixed.Direct includes were added for both symbols used in
validation_...files and symbols that are defined invalidation_.... Where it was possible redundant includes forvalidationwere removed, leading to removal of another circular dep:checkpoints->validation->checkpoints(!) - DrahtBot cross-referenced this on Aug 20, 2018 from issue util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by practicalswift
- DrahtBot cross-referenced this on Aug 20, 2018 from issue ZMQ: Small cleanups in the ZMQ code by domob1812
- DrahtBot cross-referenced this on Aug 20, 2018 from issue [refactor] Fix the chainparamsbase -> util circular dependency by Empact
- DrahtBot cross-referenced this on Aug 23, 2018 from issue Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli
- DrahtBot cross-referenced this on Aug 23, 2018 from issue Utxoscriptindex by mgrychow
- DrahtBot cross-referenced this on Aug 24, 2018 from issue refactor: Fix the chainparamsbase -> util circular dependency by Empact
- DrahtBot cross-referenced this on Aug 24, 2018 from issue Add address-based index (attempt 4?) by marcinja
- DrahtBot cross-referenced this on Aug 25, 2018 from issue build: generate MSVC project files via python script by ken2812221
- DrahtBot added the label Needs rebase on Aug 26, 2018
-
refactor: removal of cyclic dependencies between index/txindex, validation and index/base translation units 2647af7c6e
-
index/txindex -> validation -> index/txindex circular dependency removed from lint-circular-dependencies.sh 26f4a2216b
-
split of validation_globals bf5f33c353
-
some definitions moved to validation_globals.cpp, added direct includes in validation_... files b1c0c7cfa9
-
direct includes of validation_... files, checkpoints->validation->checkpoints circular dependency removed 63cb13fdfc
-
msvc build fix attempt b9bc7aca67
- mgrychow force-pushed on Aug 27, 2018
- DrahtBot removed the label Needs rebase on Aug 27, 2018
- DrahtBot cross-referenced this on Aug 30, 2018 from issue index: Create IndexRunner class for activing indexes. by jimpo
-
jimpo commented at 6:25 PM on August 30, 2018: contributor
The refactor in #13144 also breaks the cyclic dependency cited as the motivation for this PR.
That said, I still like the idea of breaking validation into multiple files. Though I prefer grouping them in a
src/validation/directory rather than just by thevalidation_file prefix. -
DrahtBot commented at 3:56 PM on August 31, 2018: contributor
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
- DrahtBot added the label Needs rebase on Aug 31, 2018
-
mgrychow commented at 6:25 PM on September 3, 2018: none
@jimpo Thanks for pointing it out. I'll close it after #13144 being merged and continue to work on splitting
validationin next PR. Still, there is a circular dependency in #14035 but I guess it may be solved within scope of that PR (interface between index and validation or something, but after #13144 merge). No rebase for now unless explicitly requested.edit: Please note that also
checkpoints -> validation -> checkpointsdependency was removed here due tovalidationsplit -
MarcoFalke commented at 8:15 PM on September 20, 2018: member
Still needs rebase, so closing for now. Let me know when you want to work on this again, so I can reopen.
- MarcoFalke closed this on Sep 20, 2018
- laanwj removed the label Needs rebase on Oct 24, 2019
- bitcoin locked this on Feb 15, 2022