backport: merge bitcoin#22903, #24753, #24831, #28240, #27012, #25713, #24971, #25047 (clang-tidy and IWYU) #6559
Conversation
kwvg
force-pushed
the
enable_iwyu
branch
2 times, most recently
from
48eff4e to
02613a5
Compare
kwvg
changed the title
clang-tidy in CI, IWYU for src/{bls,coinjoin,compat,evo,governance,init,stats})clang-tidy in CI, IWYU for src/{bls,coinjoin,compat,evo,governance,init,stats})
|
This pull request has conflicts, please rebase. |
kwvg
changed the title
clang-tidy in CI, IWYU for src/{bls,coinjoin,compat,evo,governance,init,stats})
WalkthroughThis pull request consolidates updates across the codebase that include changes to CI and Docker scripts, build configurations, and source code modernizations. The CI scripts now support an additional environment variable to enable code quality checks. Build scripts and Dockerfiles have been modified to install and configure tools such as Bear, LLVM, Clang-Tidy, and Include-What-You-Use. Multiple source files are updated to use modern C++ practices, including replacing NULL or 0 with nullptr and switching from C-style headers (e.g., <stdint.h>) to their C++ counterparts (). There are also several minor changes in parameter names, comments, and header inclusion formats in various modules such as networking, wallet operations, and RPC commands. Additionally, some outdated constants and external declarations have been removed or updated. Overall, these changes adjust the build and coding practices without altering the core functionality of the system. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between c7cb1d98d4aac64b329e8c00f0396f85fb88ec44 and 4009777. 📒 Files selected for processing (37)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (34)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (1)
ci/dash/build_src.sh (1)
68-84: Consider improving error handling in the clang-tidy section.While the script uses
set -eo pipefailfor proper error handling, the clang-tidy output filtering could be improved.Consider this alternative that captures all errors, not just those with 5 lines of context:
- ( run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error" + output=$(run-clang-tidy -quiet "${MAKEJOBS}") + if echo "$output" | grep -q "error:"; then + echo "$output" | grep -C5 "error:" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e8e6b92 and 0b42caa2e84636e9a2901b485ea87627014a013b.
📒 Files selected for processing (39)
ci/dash/build_src.sh(2 hunks)ci/test/00_setup_env.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)configure.ac(1 hunks)contrib/containers/ci/Dockerfile(3 hunks)contrib/devtools/iwyu/bitcoin.core.imp(1 hunks)src/.bear-tidy-config(1 hunks)src/.clang-tidy(1 hunks)src/bench/coin_selection.cpp(1 hunks)src/compat/byteswap.h(1 hunks)src/compat/compat.h(1 hunks)src/compat/cpuid.h(1 hunks)src/compat/endian.h(1 hunks)src/compat/stdin.cpp(1 hunks)src/governance/governance.cpp(1 hunks)src/init/bitcoin-node.cpp(1 hunks)src/init/common.cpp(1 hunks)src/llmq/signing_shares.cpp(1 hunks)src/mapport.cpp(2 hunks)src/masternode/sync.cpp(1 hunks)src/net.cpp(3 hunks)src/net_processing.cpp(1 hunks)src/netaddress.cpp(0 hunks)src/netbase.cpp(1 hunks)src/qt/bitcoin.cpp(9 hunks)src/qt/intro.cpp(1 hunks)src/randomenv.cpp(1 hunks)src/rpc/fees.cpp(1 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/quorums.cpp(0 hunks)src/rpc/signmessage.cpp(1 hunks)src/support/allocators/secure.h(1 hunks)src/test/fuzz/crypto_diff_fuzz_chacha20.cpp(1 hunks)src/torcontrol.cpp(3 hunks)src/torcontrol.h(1 hunks)src/validation.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(3 hunks)src/wallet/test/psbt_wallet_tests.cpp(0 hunks)src/wallet/wallet.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- src/netaddress.cpp
- src/rpc/quorums.cpp
- src/wallet/test/psbt_wallet_tests.cpp
✅ Files skipped from review due to trivial changes (21)
- src/compat/endian.h
- src/.bear-tidy-config
- src/init/bitcoin-node.cpp
- src/masternode/sync.cpp
- src/compat/byteswap.h
- src/rpc/signmessage.cpp
- src/compat/cpuid.h
- src/.clang-tidy
- src/compat/stdin.cpp
- src/bench/coin_selection.cpp
- src/randomenv.cpp
- src/rpc/fees.cpp
- src/test/fuzz/crypto_diff_fuzz_chacha20.cpp
- src/llmq/signing_shares.cpp
- src/wallet/wallet.cpp
- configure.ac
- src/rpc/misc.cpp
- src/net.cpp
- src/compat/compat.h
- src/wallet/test/coinselector_tests.cpp
- src/validation.cpp
🧰 Additional context used
🪛 GitHub Actions: Check Potential Conflicts
src/governance/governance.cpp
[error] 1-1: File has conflicts with multiple pull requests.
src/net_processing.cpp
[error] 1-1: File has conflicts with multiple pull requests.
🔇 Additional comments (22)
src/net_processing.cpp (3)
4766-4766: LGTM! Parameter name change improves clarityThe parameter name change from
punish_duplicate_invalidtovia_compact_blockbetter reflects the parameter's purpose and aligns with BIP 152's specification for compact block validation rules.
713-713: LGTM! Consistent parameter namingThe function signature parameter name change matches the call site change, maintaining consistency throughout the codebase.
1-6364: Please resolve conflicts with other PRsThe pipeline indicates this file has conflicts with multiple pull requests. Please rebase your changes on the latest master and resolve any conflicts to ensure clean integration.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 2760-2760: syntax error
(syntaxError)
🪛 GitHub Actions: Check Potential Conflicts
[error] 1-1: File has conflicts with multiple pull requests.
contrib/devtools/iwyu/bitcoin.core.imp (1)
1-6: LGTM! Well-structured IWYU mappings for termios headers.The configuration correctly maps implementation-specific headers to their public interface, ensuring proper encapsulation of system-specific details.
src/support/allocators/secure.h (1)
41-41: LGTM! Good modernization of null pointer syntax.Using
nullptrinstead of0improves type safety and readability, aligning with modern C++ practices.src/torcontrol.h (2)
16-20: LGTM! Good header organization.The header includes are well-organized, with system headers grouped separately from project headers.
14-14: Verify the migration from Boost.Signals2 to libevent.The removal of Boost.Signals2 in favor of libevent represents a significant architectural change. Let's verify that all asynchronous operations have been properly migrated.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining Boost.Signals2 usage rg "boost::signals2" -A 5 # Search for new event handling patterns rg "event_base" -A 5Length of output: 21538
Asynchronous Migration Verified in TorControl Module
- In src/torcontrol.h and accompanying implementation files, asynchronous operations are now managed by libevent (e.g., using
event_base_new,event_base_dispatch, andevent_base_free), with the libevent header<event2/util.h>included.- No references to Boost.Signals2 appear in the torcontrol component, confirming that network-related asynchronous tasks have been fully migrated.
- The remaining Boost.Signals2 usages in the repository (e.g., in UI signaling and callbacks) serve different roles and are not related to the asynchronous operations for Tor control.
src/init/common.cpp (1)
12-25: LGTM! Improved header organization.The header includes have been updated to better reflect actual usage, following IWYU recommendations:
- Added necessary headers for filesystem, formatting, and container operations
- Removed unused memory header
- Properly organized system vs. project headers
src/mapport.cpp (1)
198-198: LGTM! Improved type safety by usingnullptr.The changes replace integer literal
0withnullptrin UPnP function calls, which is a better practice in modern C++ as it:
- Makes the pointer nature of the parameter more explicit
- Prevents accidental implicit conversions
- Improves code readability
Also applies to: 211-211
src/qt/intro.cpp (1)
232-232: LGTM! Improved Qt widget initialization.The change from
0tonullptrfor the parent widget parameter follows Qt best practices by:
- Making it explicit that the widget has no parent
- Aligning with Qt's modern coding style
- Improving type safety in widget hierarchy management
src/netbase.cpp (1)
661-661: LGTM! Improved proxy authentication parameter handling.The change from
0tonullptrfor theauthparameter inSocks5function:
- Better represents the absence of proxy credentials
- Matches the
ProxyCredentials*parameter type- Enhances code clarity and type safety
src/torcontrol.cpp (2)
15-35: LGTM! Enhanced header organization.Added necessary headers to support:
- Random number generation (
random.h)- String formatting (
tinyformat.h)- Error checking (
util/check.h)- String utilities (
util/string.h)- Standard library components
317-317: LGTM! Improved event pointer initialization.Changed
reconnect_evinitialization from0tonullptr:
- Better represents the uninitialized event pointer
- Follows modern C++ practices
- Improves type safety
src/qt/bitcoin.cpp (1)
535-535: LGTM! Improved code safety by usingnullptrinstead of0.The changes consistently replace the use of
0withnullptrfor parent window pointers in QMessageBox constructor calls. This is a good practice in modern C++ as it makes the code's intent clearer and provides better type safety.Also applies to: 633-633, 646-646, 656-656, 666-666, 677-677, 691-691, 711-711
src/governance/governance.cpp (1)
1548-1548: LGTM! Improved comment clarity.The comment update from
/* filter = */to/* cond = */better reflects the parameter's purpose in the NodesSnapshot constructor. This change maintains consistency with similar updates across the codebase.ci/test/00_setup_env_native_multiprocess.sh (1)
12-12: LGTM! Enabled code quality checks.Setting
RUN_TIDY=trueenables additional code quality checks using clang-tidy and include-what-you-use tools.ci/dash/build_src.sh (1)
54-58: LGTM! Well-structured integration of the bear tool.The conditional block properly sets up the bear tool configuration when RUN_TIDY is enabled.
ci/test/00_setup_env.sh (1)
41-41: Add default RUN_TIDY environment variable.
IntroducingRUN_TIDY=${RUN_TIDY:-false}provides a clear, fallback value for controlling tidy checks in CI. This aligns well with the PR’s objectives of enabling conditional static analysis.contrib/containers/ci/Dockerfile (4)
24-24: Include the 'bear' static analysis tool.
Adding thebearpackage (line 24) enhances the build environment by enabling command tracing during compilation. This is useful for integrating further analysis (e.g., with clang-tidy and IWYU).
69-72: Enhance Clang/LLVM development package installation.
The additions of"libclang-${LLVM_VERSION}-dev"(line 69),"lld-${LLVM_VERSION}"(line 71), and"llvm-${LLVM_VERSION}-dev"(line 72) ensure that the necessary development files for Clang and LLVM are installed. This modification supports the static analysis tools and is consistent with aligning with upstream practices.
75-82: Update alternatives to include extended Clang tools.
The updated alternative setup (starting at line 75 with the assignment and extended in line 76) now incorporates binaries likeclang-apply-replacementsandrun-clang-tidy. This ensures that calls to these tools point to the version corresponding to${LLVM_VERSION}. The approach is sound; just verify that all paths resolve correctly in your CI environment.
138-145: Automate installation of include-what-you-use (IWYU).
The new block (lines 138–145) clones and builds IWYU from the branchclang_${LLVM_VERSION}. This integration helps maintain consistency with upstream static analysis practices. Consider verifying that the built tool meets the version expectations and that cleanup of/opt/iwyuis effective to avoid residual files.
src/torcontrol.cpp
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved error handling in message code parsing.
Changed from LocaleIndependentAtoi to ToIntegral with value_or(0) which:
- Provides better error handling for invalid inputs
- Returns a default value of 0 if parsing fails
- Uses modern C++ optional type
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the security implications of disabling hardening.
The changes disable hardening and set optimization levels to zero. While this might be necessary for the tidy checks, please confirm that:
- This is only used in development/CI environments
- The security impact of disabling hardening has been assessed
🏁 Script executed:
#!/bin/bash
# Check if these flags are only used in development/CI environments
git grep -l "disable-hardening" -- ":(exclude)ci/*"Length of output: 82
Security Alert: Hardening Flags Not Isolated to CI Environments
The shell script result shows that the --disable-hardening flag is also referenced in configure.ac and doc/build-unix.md, meaning it isn’t exclusive to the CI/development environment. This raises two concerns:
- Scope of Usage: The flag appears outside the confines of CI scripts. Please ensure that production and release builds do not inadvertently inherit these disabled hardening settings.
- Security Assessment: Confirm that the use of
--disable-hardening(and the zero optimization flags) in non-CI contexts is intentional and that any production build configurations indeed enforce proper security hardening.
It’s advisable to review the build configuration (especially in configure.ac) and related documentation (doc/build-unix.md) to determine under what conditions these flags are applied. If they are meant solely for development/CI routines, consider isolating them to avoid risks in production builds.
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/dash/build_src.sh (1)
54-58: Verify Bear config file existence.Consider adding a check for the Bear config file existence before proceeding:
if [ "${RUN_TIDY}" = "true" ]; then + if [ ! -f "src/.bear-tidy-config" ]; then + echo "Error: Bear config file not found at src/.bear-tidy-config" + exit 1 + fi MAYBE_BEAR="bear --config src/.bear-tidy-config" MAYBE_TOKEN="--" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 0eefc5618b6e33c11b48197c96af2009fcf1ab1c and c7cb1d98d4aac64b329e8c00f0396f85fb88ec44.
📒 Files selected for processing (37)
ci/dash/build_src.sh(2 hunks)ci/test/00_setup_env.sh(1 hunks)ci/test/00_setup_env_native_multiprocess.sh(1 hunks)configure.ac(1 hunks)contrib/containers/ci/Dockerfile(3 hunks)contrib/devtools/iwyu/bitcoin.core.imp(1 hunks)src/.bear-tidy-config(1 hunks)src/.clang-tidy(1 hunks)src/bench/coin_selection.cpp(1 hunks)src/compat/byteswap.h(1 hunks)src/compat/compat.h(1 hunks)src/compat/cpuid.h(1 hunks)src/compat/endian.h(1 hunks)src/compat/stdin.cpp(1 hunks)src/governance/governance.cpp(1 hunks)src/init/common.cpp(1 hunks)src/llmq/signing_shares.cpp(1 hunks)src/mapport.cpp(2 hunks)src/masternode/sync.cpp(1 hunks)src/net.cpp(3 hunks)src/net_processing.cpp(1 hunks)src/netaddress.cpp(0 hunks)src/netbase.cpp(1 hunks)src/qt/bitcoin.cpp(9 hunks)src/qt/intro.cpp(1 hunks)src/randomenv.cpp(1 hunks)src/rpc/fees.cpp(1 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/quorums.cpp(0 hunks)src/rpc/signmessage.cpp(1 hunks)src/support/allocators/secure.h(1 hunks)src/test/fuzz/crypto_diff_fuzz_chacha20.cpp(1 hunks)src/torcontrol.cpp(3 hunks)src/torcontrol.h(1 hunks)src/validation.cpp(1 hunks)src/wallet/test/psbt_wallet_tests.cpp(0 hunks)src/wallet/wallet.cpp(1 hunks)
💤 Files with no reviewable changes (3)
- src/netaddress.cpp
- src/wallet/test/psbt_wallet_tests.cpp
- src/rpc/quorums.cpp
🚧 Files skipped from review as they are similar to previous changes (33)
- src/bench/coin_selection.cpp
- src/.clang-tidy
- src/masternode/sync.cpp
- src/test/fuzz/crypto_diff_fuzz_chacha20.cpp
- src/rpc/fees.cpp
- src/rpc/signmessage.cpp
- src/compat/endian.h
- contrib/devtools/iwyu/bitcoin.core.imp
- src/compat/byteswap.h
- src/.bear-tidy-config
- ci/test/00_setup_env.sh
- src/randomenv.cpp
- src/init/common.cpp
- src/compat/stdin.cpp
- src/compat/cpuid.h
- src/qt/intro.cpp
- contrib/containers/ci/Dockerfile
- src/support/allocators/secure.h
- ci/test/00_setup_env_native_multiprocess.sh
- src/netbase.cpp
- src/torcontrol.h
- src/qt/bitcoin.cpp
- src/mapport.cpp
- src/llmq/signing_shares.cpp
- src/torcontrol.cpp
- src/compat/compat.h
- src/rpc/misc.cpp
- src/validation.cpp
- src/net_processing.cpp
- src/wallet/wallet.cpp
- src/governance/governance.cpp
- configure.ac
- src/net.cpp
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (1)
ci/dash/build_src.sh (1)
59-59: LGTM! Clean integration of Bear with make.The modification elegantly integrates Bear while maintaining the existing error handling and verbose build fallback.
ci/dash/build_src.sh
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of code quality checks.
Several improvements could make the code quality checks more robust:
- Capture all clang-tidy output, not just errors
- Use a more secure location for temporary files
- Clean up temporary files
- Consider committing IWYU changes
if [ "${RUN_TIDY}" = "true" ]; then
set -eo pipefail
cd src
- ( run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error"
+ TIDY_OUTPUT="$(mktemp -t dash_tidy.XXXXXX)"
+ ( run-clang-tidy -quiet "${MAKEJOBS}" ) | tee "${TIDY_OUTPUT}"
+ if grep -q "error:" "${TIDY_OUTPUT}"; then
+ echo "Found clang-tidy errors:"
+ grep -C5 "error:" "${TIDY_OUTPUT}"
+ rm "${TIDY_OUTPUT}"
+ exit 1
+ fi
+ rm "${TIDY_OUTPUT}"
cd ..
+ IWYU_OUTPUT="$(mktemp -t dash_iwyu.XXXXXX)"
iwyu_tool.py \
"src/compat" \
"src/init" \
"src/rpc/fees.cpp" \
"src/rpc/signmessage.cpp" \
-p . "${MAKEJOBS}" \
-- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
- |& tee "/tmp/iwyu_ci.out"
+ |& tee "${IWYU_OUTPUT}"
cd src
- fix_includes.py --nosafe_headers < /tmp/iwyu_ci.out
+ fix_includes.py --nosafe_headers < "${IWYU_OUTPUT}"
git --no-pager diff
+ rm "${IWYU_OUTPUT}"
+ # Optionally commit IWYU changes:
+ # if ! git diff --quiet; then
+ # git add -u
+ # git commit -m "style: apply IWYU fixes"
+ # fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "${RUN_TIDY}" = "true" ]; then | |
| set -eo pipefail | |
| cd src | |
| ( run-clang-tidy -quiet "${MAKEJOBS}" ) | grep -C5 "error" | |
| cd .. | |
| iwyu_tool.py \ | |
| "src/compat" \ | |
| "src/init" \ | |
| "src/rpc/fees.cpp" \ | |
| "src/rpc/signmessage.cpp" \ | |
| -p . "${MAKEJOBS}" \ | |
| -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \ | |
| |& tee "/tmp/iwyu_ci.out" | |
| cd src | |
| fix_includes.py --nosafe_headers < /tmp/iwyu_ci.out | |
| git --no-pager diff | |
| fi | |
| if [ "${RUN_TIDY}" = "true" ]; then | |
| set -eo pipefail | |
| cd src | |
| TIDY_OUTPUT="$(mktemp -t dash_tidy.XXXXXX)" | |
| ( run-clang-tidy -quiet "${MAKEJOBS}" ) | tee "${TIDY_OUTPUT}" | |
| if grep -q "error:" "${TIDY_OUTPUT}"; then | |
| echo "Found clang-tidy errors:" | |
| grep -C5 "error:" "${TIDY_OUTPUT}" | |
| rm "${TIDY_OUTPUT}" | |
| exit 1 | |
| fi | |
| rm "${TIDY_OUTPUT}" | |
| cd .. | |
| IWYU_OUTPUT="$(mktemp -t dash_iwyu.XXXXXX)" | |
| iwyu_tool.py \ | |
| "src/compat" \ | |
| "src/init" \ | |
| "src/rpc/fees.cpp" \ | |
| "src/rpc/signmessage.cpp" \ | |
| -p . "${MAKEJOBS}" \ | |
| -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \ | |
| |& tee "${IWYU_OUTPUT}" | |
| cd src | |
| fix_includes.py --nosafe_headers < "${IWYU_OUTPUT}" | |
| git --no-pager diff | |
| rm "${IWYU_OUTPUT}" | |
| # Optionally commit IWYU changes: | |
| # if ! git diff --quiet; then | |
| # git add -u | |
| # git commit -m "style: apply IWYU fixes" | |
| # fi | |
| fi |
|
This pull request has conflicts, please rebase. |
3 participants
Additional Information
While linting is generally reserved for our
arm_linuxbuild variant, utilizing IWYU (which in turns relies onbearfor the compilation database, also relied upon byclang-tidy) requires a version paired to the Clang version (source) used to build the target codebase.This means it has to be run on a build variant that uses Clang. We have opted to use
linux64_multiprocessas it the most lightweight Clang build variant available (compared to fuzz, UBSan and TSan variants which come with sanitizers tacked on).An earlier version of this PR (source) attempted to apply IWYU to Dash-specific code. This attempt was aborted because IWYU was found to behave non-deterministically.
Furthermore, while IWYU will tell you what headers to use, it will not do the following:
string.hovercstringand changing it is a manual errand)But it will do the following:
It was determined for now that we will simply take guidance from upstream on the matter and extend it to Dash code at some point in the future.
Both bitcoin#25029 and bitcoin#25013 made sure they pass IWYU, changes needed to ensure that the linter is satisfied were made in this PR.
Breaking Changes
None expected.
Checklist