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

  2. mgrychow force-pushed on Aug 11, 2018
  3. fanquake added the label Refactoring on Aug 11, 2018
  4. 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.

  5. DrahtBot cross-referenced this on Aug 11, 2018 from issue Track best-possible-headers (TheBlueMatt) by Sjors
  6. 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
  7. DrahtBot cross-referenced this on Aug 11, 2018 from issue Move src/tinyformat.h to src/tinyformat/tinyformat.h by Empact
  8. DrahtBot cross-referenced this on Aug 11, 2018 from issue Include tinyformat as a subtree by Empact
  9. DrahtBot cross-referenced this on Aug 11, 2018 from issue -masterdatadir for datadir bootstrapping by kallewoof
  10. 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.

  11. Empact commented at 5:05 PM on August 11, 2018: member

    You'll want to squash 86e36e0

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

  13. mgrychow force-pushed on Aug 20, 2018
  14. 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 (!)

  15. DrahtBot cross-referenced this on Aug 20, 2018 from issue util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool by practicalswift
  16. DrahtBot cross-referenced this on Aug 20, 2018 from issue ZMQ: Small cleanups in the ZMQ code by domob1812
  17. DrahtBot cross-referenced this on Aug 20, 2018 from issue [refactor] Fix the chainparamsbase -> util circular dependency by Empact
  18. DrahtBot cross-referenced this on Aug 23, 2018 from issue Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli
  19. DrahtBot cross-referenced this on Aug 23, 2018 from issue Utxoscriptindex by mgrychow
  20. DrahtBot cross-referenced this on Aug 24, 2018 from issue refactor: Fix the chainparamsbase -> util circular dependency by Empact
  21. DrahtBot cross-referenced this on Aug 24, 2018 from issue Add address-based index (attempt 4?) by marcinja
  22. DrahtBot cross-referenced this on Aug 25, 2018 from issue build: generate MSVC project files via python script by ken2812221
  23. DrahtBot added the label Needs rebase on Aug 26, 2018
  24. refactor: removal of cyclic dependencies between index/txindex, validation and index/base translation units 2647af7c6e
  25. index/txindex -> validation -> index/txindex circular dependency removed from lint-circular-dependencies.sh 26f4a2216b
  26. split of validation_globals bf5f33c353
  27. some definitions moved to validation_globals.cpp, added direct includes in validation_... files b1c0c7cfa9
  28. direct includes of validation_... files, checkpoints->validation->checkpoints circular dependency removed 63cb13fdfc
  29. msvc build fix attempt b9bc7aca67
  30. mgrychow force-pushed on Aug 27, 2018
  31. DrahtBot removed the label Needs rebase on Aug 27, 2018
  32. DrahtBot cross-referenced this on Aug 30, 2018 from issue index: Create IndexRunner class for activing indexes. by jimpo
  33. 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.

  34. DrahtBot commented at 3:56 PM on August 31, 2018: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  35. DrahtBot added the label Needs rebase on Aug 31, 2018
  36. 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

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

  38. MarcoFalke closed this on Sep 20, 2018

  39. laanwj removed the label Needs rebase on Oct 24, 2019
  40. bitcoin locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:55 UTC