8000 refactor: Removal of circular dependency between index/txindex, validation and index/base by mgrychow · Pull Request #13942 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

mgrychow
Copy link

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 mgrychow force-pushed the circular-dependency_removal branch from c2a55e7 to d22e36a Compare August 11, 2018 03:35
@DrahtBot
Copy link
Contributor
DrahtBot commented Aug 11, 2018 •
edited
Loading
Note to reviewers: This pull request conflicts with the following ones:

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.

This was referenced Aug 11, 2018
Closed
Closed
Closed
Closed
Closed
@Empact
Copy link
Contributor
Empact commented Aug 11, 2018 •
edited
Loading

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
Copy link
Contributor
Empact commented Aug 11, 2018

You'll want to squash 86e36e0

@Empact
Copy link
Contributor
Empact commented Aug 12, 2018

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.

@DrahtBot DrahtBot mentioned this pull request Aug 13, 2018
Closed
@mgrychow mgrychow force-pushed the circular-dependency_removal branch from 051e981 to d2caa43 Compare August 20, 2018 13:34
@mgrychow
Copy link
Author
mgrychow commented Aug 20, 2018 •
edited
Loading

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 (!)

This was referenced Aug 20, 2018
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Merged
@mgrychow mgrychow force-pushed the circular-dependency_removal branch from 93f2553 to b9bc7ac Compare August 27, 2018 18:13
@DrahtBot DrahtBot mentioned this pull request Aug 30, 2018
Closed
@jimpo
Copy link
Contributor
jimpo commented Aug 30, 2018 •
edited
Loading

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
Copy link
Contributor
Needs rebase

@mgrychow
Copy link
Author
mgrychow commented Sep 3, 2018 •
edited
Loading

@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

@maflcko
Copy link
Member
maflcko commented Sep 20, 2018

Still needs rebase, so closing for now. Let me know when you want to work on this again, so I can reopen.

@maflcko maflcko closed this Sep 20, 2018
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mgrychow @DrahtBot @Empact @jimpo @maflcko @laanwj @fanquake
0