Skip to content

backport: bitcoin/bitcoin#26905, #27724, #28482, partial #29659 - clang-tidy [perf] - #7693

Merged
knst merged 12 commits into
dashpay:developfrom
knst:bp-26905
Sep 20, 2026
Merged

knst merged 12 commits into
dashpay:developfrom
knst:bp-26905

Conversation

@knst

@knst knst commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

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:

Multiple fixes for dash code:

  • fix readability-const-return-type violations
  • fix performance-trivially-destructible violation
  • fix performance-faster-string-find violations
  • fix performance-noexcept-swap violation
  • fix performance-inefficient-vector-operation violations
  • fix performance-for-range-copy violations
  • fix modernize-use-emplace violations

How Has This Been Tested?

See CI run

Breaking Changes

Debug builds doesn't use anymore extra asserts for multi-index maps.

Checklist:

  • 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)

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.
@knst knst added this to the 24 milestone Sep 17, 2026
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@thepastaclaw

thepastaclaw commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

⚠️ DEGRADED — Final review complete — no blockers (commit d51da96) · triage: normal · stand-in models (primary models out of quota)

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 6258c484-3b3e-47e0-bea4-0a701bc49b7a

📥 Commits

Reviewing files that changed from the base of the PR and between e435fd7 and d51da96.

📒 Files selected for processing (1)
  • src/.clang-tidy

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The 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 GovernanceSigner destructor and adds noexcept to BlsCheck::swap. The summarized call-site changes preserve runtime behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Merge Risk: ⚪ Minimal · up to d51da

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the backported Bitcoin changes and their clang-tidy performance focus, which matches the main changeset.
Description check ✅ Passed The description directly explains the clang-tidy compatibility fixes, listed backports, affected violation categories, testing, and the debug-build behavior change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ DEGRADED — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: 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-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. 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-astra unavailable: 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-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-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); agent phase1-reviewer, gemini-3.8-flash-high — backport-reviewer (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — dash-core-commit-history (completed, effort high); agent phase1-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 for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — backport-reviewer (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — dash-core-commit-history (completed, effort high); agent phase2-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 inside while (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.

Comment thread src/.clang-tidy Outdated
Comment on lines +24 to +25
# --enable-debug defines BOOST_MULTI_INDEX_ENABLE_SAFE_MODE, which makes
# mempool iterators non-trivially copyable in that configuration only

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💬 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.

Suggested change
# --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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

valid issue ; had been done before including 27724 to the scope.

Cleaned up

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 thepastaclaw added the pastaclaw:commented thepastaclaw's latest review was comment-only label Sep 18, 2026
knst and others added 10 commits September 18, 2026 23:19
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 thepastaclaw removed the pastaclaw:commented thepastaclaw's latest review was comment-only label Sep 18, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: 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-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. 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-astra unavailable: 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-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-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); agent phase1-reviewer, gemini-3.8-flash-high — backport-reviewer (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — dash-core-commit-history (completed, effort high); agent phase1-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 for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — backport-reviewer (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — dash-core-commit-history (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — backport-reviewer (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — dash-core-commit-history (completed, effort high); agent phase2-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.

@thepastaclaw thepastaclaw added the pastaclaw:commented thepastaclaw's latest review was comment-only label Sep 18, 2026
@knst
knst merged commit 40d8233 into dashpay:develop Sep 20, 2026
50 of 52 checks passed
@thepastaclaw thepastaclaw removed the pastaclaw:commented thepastaclaw's latest review was comment-only label Sep 20, 2026
@knst
knst deleted the bp-26905 branch September 20, 2026 19:02
knst pushed a commit that referenced this pull request Sep 21, 2026
clang-tidy's performance-inefficient-vector-operation check flags these four loops. They became hard errors when #7693 backported bitcoin#26905, which replaced the explicit WarningsAsErrors allowlist in src/.clang-tidy with '*'.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
knst added a commit that referenced this pull request Sep 21, 2026
cf70026 lint: drop top-level const from ExpectedType's return type (pasta)
b7ca886 lint: reserve vector capacity before push_back loops (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  `develop` currently fails the `linux64_multiprocess-build / Build source` → "Run linters" step (`ci/dash/lint-tidy.sh`).

  #7693 backported bitcoin#26905, which replaced the explicit `WarningsAsErrors` allowlist in `src/.clang-tidy` with `WarningsAsErrors: '*'` (and added a matching `-warnings-as-errors=-clang-diagnostic-old-style-cast,-google-readability-casting` to `ci/dash/lint-tidy.sh` so the C-style-cast checks stay advisory). Every other enabled check is now a hard error, which promoted five pre-existing diagnostics from warnings to build failures:

  ```
  llmq/quorumproofs.cpp:220:38: error: 'push_back' is called inside a loop [performance-inefficient-vector-operation]
  llmq/quorumproofs.cpp:405:43: error: 'push_back' is called inside a loop [performance-inefficient-vector-operation]
  rpc/quorums.cpp:1569:25:     error: 'push_back' is called inside a loop [performance-inefficient-vector-operation]
  rpc/output_script.cpp:133:17: error: 'push_back' is called inside a loop [performance-inefficient-vector-operation]
  rpc/util.cpp:890:1:          error: return type 'const std::optional<UniValue::VType>' is 'const'-qualified at the top level [readability-const-return-type]
  ```

  The three quorum-proof sites came in with #7107; the two `rpc/` sites are older code that was only ever a warning before.

  The failure is not visible on PRs branched before #7693 merged, because CI checks out the PR head and therefore uses that branch's `.clang-tidy`. It shows up on any PR whose merge base is at or after #7693 — e.g. https://github.com/dashpay/dash/actions/runs/35530545254/job/106137443857.

  ## What was done?

  - `hashes.reserve(...)` / `leaves.reserve(...)` / `pubkeys.reserve(...)` ahead of the four flagged loops. The sizes are known up front in every case, so this is also a small genuine improvement.
  - Dropped the top-level `const` from `ExpectedType`'s by-value return type in `src/rpc/util.cpp`. It never conferred anything.

  No behavior changes.

  ## How Has This Been Tested?

  Reproduced the failure and verified the fix locally with clang-tidy 19 (same major version as CI's `LLVM_VERSION`), running the repo's `src/.clang-tidy` config against a compilation database for the four affected translation units:

  - Before: the two `llmq/quorumproofs.cpp` errors reproduce verbatim, including the `[performance-inefficient-vector-operation,-warnings-as-errors]` tag from CI.
  - After: zero `performance-inefficient-vector-operation` and zero `readability-const-return-type` diagnostics across all four files.

  All four translation units compile clean (`rpc/util.cpp`, `rpc/output_script.cpp`, `rpc/quorums.cpp`, `llmq/quorumproofs.cpp`) on an `aarch64-apple-darwin` depends build. Full CI will cover the rest.

  ## Breaking Changes

  None.

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

ACKs for top commit:
  knst:
    utACK cf70026

Tree-SHA512: f8394e31839ff2fdca2da7d515a29b7e28f252e457be8a55678098e79abfdc25d36aaa05cdf78bc8edd066c4ef354cc0d7cf814873e3e92dfa245c195a34b96c
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.

3 participants