Conversation
lint-tidy.sh appends clang-diagnostic-old-style-cast and google-readability-casting via -checks and relies on them staying warnings: lint-cstyle-casts.py only fails the job for casts in files listed in test/util/data/non-backported.txt and filters everything else out. With WarningsAsErrors: '*' from bitcoin#26905 every cast in upstream code becomes an error, clang-tidy exits non-zero and run-clang-tidy fails the job before the filter gets to decide. -warnings-as-errors is appended to the value from .clang-tidy, so negating the two checks on the command line keeps the config file identical to upstream.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change updates clang-tidy and Boost multi_index build settings. It replaces temporary container construction with direct emplacement, adds capacity reservations, and avoids selected copies across core, GUI, RPC, and wallet code. It removes the explicit Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The change contains validated build configuration and behavior-preserving C++ cleanups, with no established regression requiring resolution before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 7.32% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 41 functions across 36 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Backport stack for clang-tidy config and boost safe-mode handling is faithful with no behavioral or consensus impact. The only in-scope issue is a stale .clang-tidy comment invalidated by the safe-mode removal. A pre-existing benchmark index bug is noted separately as out-of-scope.
💬 1 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: backport-reviewer); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 4: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: backport-reviewer); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Large cross-cutting but mechanical clang-tidy performance/style fixes across 40 files with no change to consensus, crypto, or funds-movement logic. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— backport-reviewer (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— dash-core-commit-history (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 100% left, 5h 100% left - Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — backport-reviewer (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/.clang-tidy`:
- [NITPICK] src/.clang-tidy:24-25: Stale comment still claims --enable-debug defines safe mode
Commit 47eabb8dc1 removes the automatic -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE from configure.ac for --enable-debug/--enable-fuzz; only ci/test/00_setup_env_native_qt5.sh now enables it explicitly. The comment above performance-for-range-copy.AllowedTypes still states --enable-debug defines the macro, which misleads future readers about when the safe_iterator allowlist entry applies.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Loop index reset inside while-loop in BLS_Verify_BatchedParallel benchmark — In src/bench/bls.cpp,
size_t i = 0;is insidewhile (futures.size() < 10000), so it resets every iteration and the benchmark always verifies index 0. Pre-existing since file creation; this PR only changed make_pair to emplace_back, so it is outside this PR's scope.- Follow-up: Move
size_t i = 0;outside the while loop in a separate benchmark maintenance commit.
- Follow-up: Move
| # --enable-debug defines BOOST_MULTI_INDEX_ENABLE_SAFE_MODE, which makes | ||
| # mempool iterators non-trivially copyable in that configuration only |
There was a problem hiding this comment.
💬 Nitpick: Stale comment still claims --enable-debug defines safe mode
Commit 47eabb8 removes the automatic -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE from configure.ac for --enable-debug/--enable-fuzz; only ci/test/00_setup_env_native_qt5.sh now enables it explicitly. The comment above performance-for-range-copy.AllowedTypes still states --enable-debug defines the macro, which misleads future readers about when the safe_iterator allowlist entry applies.
| # --enable-debug defines BOOST_MULTI_INDEX_ENABLE_SAFE_MODE, which makes | |
| # mempool iterators non-trivially copyable in that configuration only | |
| # BOOST_MULTI_INDEX_ENABLE_SAFE_MODE (enabled explicitly in some CI jobs) makes | |
| # mempool iterators non-trivially copyable in that configuration only |
source: muse-spark-1.3-contributor (phase2-reviewer: backport-reviewer, dash-core-commit-history)
There was a problem hiding this comment.
valid issue ; had been done before including 27724 to the scope.
Cleaned up
There was a problem hiding this comment.
Resolved (re-reviewed at d51da968): Thanks for cleaning it up — verified src/.clang-tidy at this head no longer contains the stale comment or allowlist entry.
thepastaclaw
added
the
pastaclaw:commented
BACKPORT NOTE: LLVM/Clang 19 is already used everywhere ---- 8735e2c ci: use LLVM/Clang 17 in tidy job (fanquake) ce46b68 ci: use LLVM 17.0.2 in MSAN jobs (fanquake) Pull request description: Also update MSAN to use 17.0.2. Related to bitcoin#28465. ACKs for top commit: maflcko: lgtm ACK 8735e2c Tree-SHA512: 74452b95326cf065afe8332dc1b5b8e5ac12c8fe05c278a1cee017f87a7f7e0cdb8cac5e39d718c8ef587c8ee229bbaadd847df9f191313d41c5cdcab45e7c76 Co-authored-by: fanquake <fanquake@gmail.com>
BACKPORT NOTE: missing changes due to non-backported taproot yet ---- 636c986 ci: Bump `TIDY_LLVM_V` (Hennadii Stepanov) Pull request description: This PR switches to the latest [IWYU 0.22](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.22), which is compatible with Clang 18. ACKs for top commit: fanquake: ACK 636c986 Tree-SHA512: 78ce89244c5e487dd1be8b4bd2ca6f06d19b04b78289ebc21985110574053545dcce5eb622edf2bede2cf7bb58360170e976d30a4484a127d34dd17b1c604e9c Co-authored-by: fanquake <fanquake@gmail.com>
…bug mode 59c8944 build: disable boost multi index safe mode (willcl-ark) Pull request description: Fixes bitcoin#27586 Disable boost multi index safe mode by default when configuring with --enable-debug. This option can cause transactions to take a long time to be accepted into the mempool under certain conditions; iterator destruction takes O(n) time vs O(1) as they are stored in a singly linked list. See 27586 and the [boost docs](https://www.boost.org/doc/libs/1_58_0/boost/multi_index/detail/safe_mode.hpp) for more information. Re-enable it on the CI builds which previously had it enabled. Re-enable it on the msan fuzz task so that we have fuzz tasks testing with it enabled and disabled in this repo. ACKs for top commit: hebasto: ~ACK 59c8944~ fanquake: ACK 59c8944 Tree-SHA512: ed654f63dbebdd02e4414d1f81147d92a4d490dbb5a2e0376858e3129097645f3a2df45191d6b40c410a76e803b0d28796d1a01c1d2fd995b94e8b7eb3949027 Co-authored-by: fanquake <fanquake@gmail.com>
…heck names 06fc293 refactor: Remove duplication of clang-tidy's check names (Hennadii Stepanov) Pull request description: This PR removes duplication of `clang-tidy`'s check names. No behavior change. Split up from bitcoin#26642 as [requested](bitcoin#26642 (comment)). ACKs for top commit: fanquake: ACK 06fc293 Tree-SHA512: a21bef3d7d7201e14565b526af2eae7a90cf0f792803704a80a70a4c78f07ef2a2eef6a8dced80361efbf13291ecccb0977378b9532fc30970a2070426e4d82c Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Backport of upstream clang-tidy changes with Dash-specific fixes is clean at this head; the stale safe-mode comment is gone and no new defects were found.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: backport-reviewer); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 4: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: backport-reviewer); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: backport-reviewer); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Cross-cutting mechanical clang-tidy and build-config cleanups across 40 files with no consensus, funds-movement, or crypto behavior change. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— backport-reviewer (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— dash-core-commit-history (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 100% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — backport-reviewer (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — dash-core-commit-history (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — backport-reviewer (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
2 participants
Issue being fixed or feature implemented
Just backporting bare bitcoin#26095 doesn't work, due to newer version of clang, multiple dash specific code.
What was done?
Backports:
clang-tidy's check names bitcoin/bitcoin#26905TIDY_LLVM_Vbitcoin/bitcoin#29659Multiple fixes for dash code:
How Has This Been Tested?
See CI run
Breaking Changes
Debug builds doesn't use anymore extra asserts for multi-index maps.
Checklist: