refactor: removed some exceptions for circular dependencies over evo/mnhftx, evo/chainhelper and dsnotificationinterface #6728
Conversation
WalkthroughThis set of changes removes the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/instantsend.cpp (1)
30-32: Update the comment to accurately reflect the dependency being broken.The comment states "Forward declaration to break dependency over node/transaction.h" but according to the AI summary, the
GetTransactionBlockfunction was moved TOnode/transaction.h. The comment should clarify that this avoids includingnode/transaction.hdirectly to prevent potential circular dependencies.-// Forward declaration to break dependency over node/transaction.h +// Forward declaration to avoid including node/transaction.h and prevent circular dependencies std::pair<CTransactionRef, uint256> GetTransactionBlock(const uint256& hash, const CTxMemPool* const mempool);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/evo/chainhelper.cpp(0 hunks)src/evo/chainhelper.h(0 hunks)src/evo/deterministicmns.cpp(2 hunks)src/governance/object.cpp(2 hunks)src/llmq/chainlocks.cpp(1 hunks)src/llmq/instantsend.cpp(1 hunks)src/node/transaction.cpp(1 hunks)src/node/transaction.h(1 hunks)src/validation.cpp(1 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (3)
- test/lint/lint-circular-dependencies.py
- src/evo/chainhelper.h
- src/evo/chainhelper.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/governance/object.cpp
[error] 10-21: Clang format differences found. The file does not adhere to the expected code style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (10)
src/llmq/chainlocks.cpp (1)
26-27: LGTM! Clean forward declaration approach.The forward declaration correctly breaks the circular dependency on
node/transaction.hwhile maintaining the required functionality. The explanatory comment is helpful and the function signature matches the usage pattern.src/node/transaction.h (1)
60-66: LGTM! Well-documented function declaration.The new
GetTransactionBlockfunction declaration is properly documented and serves its purpose as a wrapper to break circular dependencies. The function signature is consistent with its usage in the calling code.src/validation.cpp (1)
120-121: Appropriately forward declare GetTransactionBlock to break dependency
The signature matches the implementation innode/transaction.cpp/node/transaction.h. All referenced types (CTransactionRef,CTxMemPool,uint256) are already available via existing includes. This cleanly removes the need to includeevo/chainhelper.hhere and breaks the circular dependency.src/governance/object.cpp (2)
18-18: LGTM: Appropriate header inclusion for direct txindex access.The inclusion of
<index/txindex.h>is necessary for the direct usage ofg_txindexin the refactored code.
456-463: LGTM: Direct txindex usage with proper error handling.The refactored code correctly handles the case where
g_txindexis not available by returning an error. This approach is equivalent to the previousGetTransactionBlockfunction but eliminates the dependency onevo/chainhelper.h.However, there's a formatting issue reported by the pipeline that needs to be addressed.
The pipeline reports formatting issues in this file. Please run clang-format to fix the code style:
#!/bin/bash # Check formatting issues in the file clang-format --dry-run src/governance/object.cppsrc/evo/deterministicmns.cpp (2)
29-31: LGTM: Proper forward declarations to break header dependencies.The forward declarations for
CTxMemPoolandGetTransactionBlockeffectively break the dependency onnode/transaction.hwhile maintaining functionality.
56-56: LGTM: Function call updated to match new signature.The updated call to
GetTransactionBlockcorrectly passesnullptrfor the mempool parameter, which is appropriate since this code doesn't have access to a mempool instance.src/node/transaction.cpp (1)
152-157: LGTM: Well-implemented wrapper function with proper fast-fail logic.The new
GetTransactionBlockfunction effectively wraps the existingGetTransactionfunction and provides appropriate fast-fail behavior when neitherg_txindexnor mempool are available. The implementation maintains the same behavior as the original function while centralizing transaction retrieval logic in the node layer.src/llmq/instantsend.cpp (2)
580-580: The function usage is consistent with the forward declaration.All three usage sites correctly use structured binding and match the expected signature of the forward-declared function. The refactoring successfully maintains the existing functionality while breaking the circular dependency.
Also applies to: 637-637, 1016-1016
30-31: ```shell
#!/bin/bashCorrectly search for all declarations/definitions of GetTransactionBlock across the repo
echo "=== Matches in headers and sources for GetTransactionBlock (escaped parentheses) ==="
rg -n "GetTransactionBlock\(" -H .echo -e "\n=== Specific search in src/node/transaction.* ==="
rg -n "GetTransactionBlock" -H src/node/transaction.*</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Not sure I like this; feels more like "suppressing" a circular dependency rather than breaking it, as we still have a logical circular dependency, but just we've hacked around it so the linter is happy. Might make re-compiles a bit better, but feels there should be a more correct way to properly resolve this circular dependency. |
no, this PR on its own; I just noticed while doing unrelated changes.
yes, but compilation will cause linkage error and it's easy to fix all of them at once.
yes, that's true; real dependency over the function But I'd still created this PR, because having these exceptions inside lint-circular-dependencies.py is a positive reinforcement to make even more circular dependencies over And thirdly, please notice that it's now so much hacky, because this approach is already used in Bitcoin Core, for example:
(that's just few examples that I found right away in seconds) |
knst
force-pushed
the
fix-circular-chainhelper
branch
from
f3a9cb6 to
f6895c1
Compare
knst
changed the title
knst
changed the title
|
pls consider 5e4685a |
Added + couple extra changes to follow them. |
knst
force-pushed
the
fix-circular-chainhelper
branch
from
2140ec0 to
454681a
Compare
knst
changed the title
knst
changed the title
|
This pull request has conflicts, please rebase. |
…s and dsnotificationinterface
knst
force-pushed
the
fix-circular-chainhelper
branch
from
045cf03 to
1f4a5a6
Compare
9aa039c refactor: removed multiple unused includes; added some must-have headers (Konstantin Akimov) b389d0b refactor: drop several circular dependencies by removing unused headers (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Some includes are leftover after code movements, refactors or feature edits. Some includes meant to be, but included indirectly. ## What was done? I run iwyu [include what you use] analyzer and got 15k lines diff which touches 800 files. It's a bit too big diff and not quite useful! See full set of changes here: https://github.com/knst/dash/tree/refactor-iwyu-all Though, it helped to remove 11 exception for circular dependencies linter, so, this PR has a small sub-set of original changes that are split to 2 commits: - breakage of circular dependencies (some includes are removed) - compilation time improvements by removing obviously unused includes from Dash specific code ## How Has This Been Tested? Here're steps to run iwyu analyser: make clean bear -- make -j14 iwyu_tool -p compile_commands.json -j14 > out.txt fix_include See test/lint/lint-circular-dependencies.py, it removed 10 exception for circular dependencies. Originally has been 11 of them, but one has been found before manually, and merged as #6728, so, 10 only left for this PR. Compilation time is reduced insignificantly, only 1 minute out of 98 minutes (assuming single core compilation). ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 9aa039c kwvg: utACK 9aa039c Tree-SHA512: 63b131b993d09b1911bdca73337b6ad40717229db4beca4a55171434bac861a12dc94d698a800b62914baab4e9ad3515eaa01dc79cf3fc3f2c48ca6b76e7cf68
4 participants
Issue being fixed or feature implemented
Addressed some exception from
test/lint/lint-circular-dependencies.pyWhat was done?
See commits descriptions.
How Has This Been Tested?
Updated these exception in linter:
Breaking Changes
N/A
Checklist: