refactor: Removal of circular dependency between index/txindex, validation and index/base

mgrychow commented at 2:48 am on August 11, 2018: none

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.

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: member

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

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> in validation_block_utils.h

Every .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 to validation_globals.cpp would be more consistent with that file’s purpose of containing and presenting the globals. One idea is to rename validation_block_utils.h to chainstate.h and move g_chainstate, mapBlockIndex and chainActive out to it.

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.h set in validation were moved to validation_globals.cpp where possible, but please note that there are still txmempool->validation->txmempool and policy/fees->policy/policy->validation->policy/fees circular dependencies that prevent moving mapBlockIndex, feeEstimator and mempool as it would create another CD. Similarly, g_chainstate cannot yet be moved to file included by validation, because CChainState uses ConnectTrace that uses MemPoolRemovalReason from txmempool, which includes validation (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 in validation_.... Where it was possible redundant includes for validation were removed, leading to removal of another circular dep: checkpoints->validation->checkpoints (!)

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

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 the validation_ file prefix.

DrahtBot commented at 3:56 pm on August 31, 2018: member

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 validation in 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 -> checkpoints dependency was removed here due to validation split

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

DrahtBot locked this on Feb 15, 2022

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942