Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Oct 16, 2025

Issue being fixed or feature implemented

Some files has been missing when added to iwyu linter list.

What was done?

Backport bitcoin#25645, bitcoin#25668 missing fixes from bitcoin#24974, bitcoin#25694, bitcoin#25254 and related fixes to apply linter's messages.

How Has This Been Tested?

Revised output of CI iwuy

Breaking Changes

N/A

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

faf98ae Remove unused includes in rpc/fees.cpp (MacroFake)
1111dde Remove unused includes from dbwrapper.h (MacroFake)
fa77fdd Add missing includes (MacroFake)
fa869ce Add missing includes to node/chainstate (MacroFake)

Pull request description:

  Unused includes are confusing, but also cause unrelated compile errors when the unused includes were to be removed.

  Fix that by adding the missing includes where they are needed and then remove them where they are not needed. This is also checked by iwyu.

ACKs for top commit:
  hebasto:
    ACK faf98ae, I have reviewed the code and it looks OK, I agree it can be merged.
  jarolrod:
    Code Review ACK bitcoin@faf98ae

Tree-SHA512: 75f3c6e6f6ecf8a98233e1a1463c75ca4e0eb3ec341150d274141072fe95413a3c2ec6386d1c527899cc63d43f63f5eb5991509847412773362808ddfb1bb435
@github-actions
Copy link

github-actions bot commented Oct 16, 2025

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Oct 16, 2025

Walkthrough

This pull request systematically updates and reorganizes include directives across the codebase, primarily expanding the set of files analyzed by the include-what-you-use (IWYU) tool during CI checks. Changes include adding <util/system.h> and other utility header dependencies to multiple translation units, reordering existing includes, and removing unused headers. Additionally, the LoadChainstate() function signature in src/node/chainstate.cpp has been expanded to accept additional parameters including governance managers, masternode managers, feature toggles, and configuration flags. No changes to runtime logic or control flow are introduced outside of the signature modification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Rationale: The diff consists primarily of homogeneous include-directive modifications repeated across ~17 files, which individually require minimal review effort. However, the heterogeneous elements present—particularly the significant LoadChainstate() signature expansion and scattered include removals—demand separate reasoning for each variation. The signature change in src/node/chainstate.cpp introduces a substantially broader parameter surface with dependencies on multiple subsystems (governance, masternode, EVODB, LLMQ context, feature toggles, and consensus parameters), requiring careful verification of callers and initialization semantics. While most individual changes are mechanical, the mix of patterns and the one complex API change justify a moderate complexity assessment.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "backport: bitcoin#25645, bitcoin#25668 fix iwyu on node/chainstate, dbwrapper.h and related fixes" is directly related to the main objective of the changeset. The title accurately conveys that this is a backport of IWYU (Include What You Use) fixes targeting specific files (node/chainstate, dbwrapper.h), which aligns with the raw summary showing extensive include directive modifications across multiple translation units and the CI lint-tidy script expansion. The title is specific, clear, and sufficiently descriptive for a developer to understand the primary change without ambiguity.
Description Check ✅ Passed The pull request description is directly related to the changeset provided. The author describes the issue (files missing from the IWYU linter list), the action taken (backporting Bitcoin Core fixes and applying IWYU linter messages), and the testing approach (CI IWYU output). These descriptions align with the observable changes in the raw summary, which includes modifications to the CI lint-tidy script to expand IWYU analysis scope and numerous include header adjustments across multiple files to satisfy linter requirements. The description is specific and meaningful, mentioning IWYU, CI checks, and concrete Bitcoin PR references, rather than generic or vague terminology.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

MacroFake and others added 5 commits October 17, 2025 01:20
fad3c58 refactor: Fix iwyu on node/chainstate (MacroFake)

Pull request description:

  Fix the CI warning on master: https://cirrus-ci.com/task/5398182703136768?logs=ci#L7020

ACKs for top commit:
  fanquake:
    ACK fad3c58 - could do chain.h

Tree-SHA512: 94f6ea0b3d9667863a4217b65bd1b9e07c65bdb566378faf0727bae5eb38d2d527ecae0c39efdda740b7ab7c8269141437ffbcb470cca7d559f09b8ee132d101
@knst knst changed the title backport: bitcoin/bitcoin#25645 Remove unused includes from dbwrapper.h and related fixes for iwuy backport: bitcoin/bitcoin#25645, #25668 fix iwyu on node/chainstate, dbwrapper.h and related fixes Oct 16, 2025
@knst knst added this to the 23 milestone Oct 16, 2025
@knst
Copy link
Collaborator Author

knst commented Oct 16, 2025

I looked more to bitcoin's implementation of iwyu - it's never triggers error; moreover at some moment they just added all src/ to the iwyu linter and just use it for information as possible refactorings, but not like CI that must succeed.

So, iwyu linter stays just advisory, not mandatory

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 79f7424

Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 79f7424

@PastaPastaPasta PastaPastaPasta merged commit 92478f6 into dashpay:develop Oct 21, 2025
37 of 39 checks passed
@PastaPastaPasta PastaPastaPasta modified the milestones: 23, 23.1 Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants