Skip to content

backport: bitcoin#23703, #24860, #25642, #25650, #25760, #25829, #25869, #26589 #6916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

backport: bitcoin#23703, #24860, #25642, #25650, #25760, #25829, #25869, #26589 #6916

wants to merge 10 commits into from

Conversation

@knst
Copy link
Collaborator

@knst knst commented Oct 24, 2025 •
edited
Loading

What was done?

Regular backports from Bitcoin Core v24

How Has This Been Tested?

  1. Run unit & functional tests.
  2. Run test/get_previous_releases.py -b - it works

Breaking Changes

N/A

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

@knst knst added this to the 23.1 milestone Oct 24, 2025
@github-actions
Copy link

github-actions bot commented Oct 24, 2025 •
edited
Loading

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

Please coordinate with the authors of these PRs to avoid merge conflicts.

@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025 •
edited
Loading

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

The PR makes cross-cutting changes: CI scripts switch from a specific previous-release list to a DOWNLOAD_PREVIOUS_RELEASES flag and update invocation of get_previous_releases.py; packaging adds share/rpcauth to distributions and installer; REST mempool endpoints are consolidated into a single /rest/mempool/ handler with sub-path validation; key derivation gains max-depth guards and [[nodiscard]] annotations; OUTPUT_TYPES no longer exposes UNKNOWN; miniscript is refactored to use Fragment and context-aware optional APIs; descriptor and HD derivation propagate Derive failures instead of asserting; tests and fuzz targets updated accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant REST_Dispatcher as REST Dispatcher
    participant rest_mempool as rest_mempool Handler
    participant MempoolJSON as JSON Builder

    rect rgb(220,240,220)
      note left of REST_Dispatcher: New routing
      REST_Dispatcher->>rest_mempool: route "/rest/mempool/" requests
    end

    Client->>REST_Dispatcher: GET /rest/mempool/info
    REST_Dispatcher->>rest_mempool: Call with str_uri_part="info"
    rest_mempool->>rest_mempool: Validate sub-path ("info"|"contents")
    alt valid
        rest_mempool->>MempoolJSON: call MempoolInfoToJSON
        MempoolJSON-->>rest_mempool: JSON
        rest_mempool-->>Client: 200 OK + JSON
    else invalid
        rest_mempool-->>Client: 400 Bad Request ("invalid URI format")
    end

    Client->>REST_Dispatcher: GET /rest/mempool/contents
    REST_Dispatcher->>rest_mempool: Call with str_uri_part="contents"
    %% same successful path as above
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Files/areas needing extra attention:
    • src/script/miniscript.cpp/.h — widespread API changes: Fragment substitution, context threading, optional return semantics.
    • src/script/descriptor.cpp and src/wallet/hdchain.cpp — propagation of Derive failures and new exception wrapper; verify correctness across code paths.
    • src/pubkey.cpp/.h and src/key.cpp/.h — max-depth guards and [[nodiscard]] annotations; ensure callers handle return values.
    • src/rest.cpp — unified handler behavior and routing table updates.
    • test/get_previous_releases.py and CI scripts (ci/test/00_setup_env_native_qt5.sh, ci/test/05_before_script.sh) — changed environment variables and argument handling; confirm CLI semantics and help text.
    • Packaging/installer changes (contrib/guix/libexec/build.sh, share/setup.nsi.in) — inclusion of rpcauth and uninstall behavior.
    • Test updates (src/test/*, fuzz targets) — ensure test expectations align with new APIs and removed fuzz source.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% 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 title "backport: bitcoin#23703, bitcoin#24860, bitcoin#25642, bitcoin#25650, bitcoin#25760, bitcoin#25829, bitcoin#25869, bitcoin#26589" accurately identifies the changeset as a series of Bitcoin Core backports and specifically cites the upstream sources. This is directly related to the actual changes in the pull request, which comprise multiple backported features and fixes spanning key derivation depth limits, miniscript refactoring, REST API consolidation, output types, wallet improvements, and testing infrastructure. While the title lists PR identifiers rather than describing specific functional changes, it is concise, specific (not generic like "misc updates"), and clearly indicates the nature and scope of the work. For a multi-component backport PR, citing upstream sources is legitimate and provides reviewers with a path to understand the original context.
Description Check ✅ Passed The description is clearly related to the changeset. It states "Regular backports from Bitcoin Core v24" which directly corresponds to the multiple upstream Bitcoin Core changes present throughout the pull request. The description also explains the testing approach (unit and functional tests, plus a specific command test), which relates to the testing infrastructure changes visible in the changeset. While relatively brief, the description conveys meaningful information about the purpose and validation of the changes without being vague or generic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

coderabbitai[bot]
coderabbitai bot reviewed Oct 24, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/univalue/lib/univalue.cpp (1)

104-143: Critical: Do not modify vendored dependencies.

According to the coding guidelines, changes to vendored dependencies in src/univalue are prohibited under any circumstances. These modifications to the UniValue library API must be contributed upstream to the univalue project first, then the vendored copy should be updated via the standard vendoring process.

Based on coding guidelines.

test/get_previous_releases.py (1)

170-176: Broken checksum fallback after struct change (always misses).

if tarball in SHA256_SUMS.values() no longer works since values are dicts. This misclassifies “known tarball but wrong hash” as “unknown version”. Fix by checking meta['tarball'].

Apply:

-    if tarballHash not in SHA256_SUMS or SHA256_SUMS[tarballHash]['tarball'] != tarball:
-        if tarball in SHA256_SUMS.values():
+    if tarballHash not in SHA256_SUMS or SHA256_SUMS[tarballHash]['tarball'] != tarball:
+        if any(v['tarball'] == tarball for v in SHA256_SUMS.values()):
             print("Checksum did not match")
             return 1
🧹 Nitpick comments (4)
src/wallet/hdchain.cpp (1)

132-137: Consider enhancing the error message with derivation context.

The generic error message "Could not derive extended key" doesn't indicate which derivation step or index failed, making debugging more difficult.

Consider passing additional context to make the error message more informative:

-//! Try to derive an extended key, throw if it fails.
-static void DeriveExtKey(CExtKey& key_in, unsigned int index, CExtKey& key_out) {
+//! Try to derive an extended key, throw if it fails.
+static void DeriveExtKey(CExtKey& key_in, unsigned int index, CExtKey& key_out, const std::string& step = "") {
     if (!key_in.Derive(key_out, index)) {
-        throw std::runtime_error("Could not derive extended key");
+        throw std::runtime_error(strprintf("Could not derive extended key%s at index %u", 
+                                          step.empty() ? "" : " for " + step, index));
     }
 }

Then update the call sites to include context:

DeriveExtKey(masterKey, 44 | 0x80000000, purposeKey, "purpose");
DeriveExtKey(purposeKey, Params().ExtCoinType() | 0x80000000, cointypeKey, "coin_type");
// etc.
test/README.md (1)

106-106: Add language specifier to code block for consistency.

The markdown linter flags that this fenced code block should have a language specified. Consider adding shell or bash after the opening triple backticks for consistency with other code blocks in this file.

Apply this diff:

-```
+```shell
 test/get_previous_releases.py -b

</blockquote></details>
<details>
<summary>ci/test/00_setup_env_native_qt5.sh (1)</summary><blockquote>

`17-17`: **Confirm downstream flag handling and job scope.**

Switching to DOWNLOAD_PREVIOUS_RELEASES="true" looks fine. Please verify 05_before_script.sh gates downloads on this flag and that this is enabled only for backcompat jobs to avoid unnecessary network I/O in other CI lanes.

</blockquote></details>
<details>
<summary>src/test/fuzz/miniscript.cpp (1)</summary><blockquote>

`156-167`: **Optional: reuse FuzzInit for miniscript_script target.**

Not required, but adding `.init = FuzzInit` keeps both targets’ environments consistent. 

```diff
-FUZZ_TARGET(miniscript_script)
+FUZZ_TARGET(miniscript_script, .init = FuzzInit)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e71a9a and 905ff0a.

📒 Files selected for processing (64)
  • ci/dash/lint-tidy.sh (1 hunks)
  • ci/test/00_setup_env_native_qt5.sh (1 hunks)
  • ci/test/05_before_script.sh (1 hunks)
  • contrib/guix/libexec/build.sh (1 hunks)
  • share/setup.nsi.in (2 hunks)
  • src/.clang-tidy (1 hunks)
  • src/Makefile.test.include (1 hunks)
  • src/bench/wallet_loading.cpp (0 hunks)
  • src/bitcoin-cli.cpp (1 hunks)
  • src/bitcoin-tx.cpp (1 hunks)
  • src/blockfilter.cpp (1 hunks)
  • src/coins.cpp (1 hunks)
  • src/core_read.cpp (1 hunks)
  • src/init.cpp (1 hunks)
  • src/key.cpp (1 hunks)
  • src/key.h (2 hunks)
  • src/logging.cpp (1 hunks)
  • src/netbase.cpp (3 hunks)
  • src/node/blockstorage.cpp (1 hunks)
  • src/outputtype.cpp (1 hunks)
  • src/outputtype.h (1 hunks)
  • src/pubkey.cpp (1 hunks)
  • src/pubkey.h (2 hunks)
  • src/qt/splashscreen.cpp (1 hunks)
  • src/rest.cpp (2 hunks)
  • src/rpc/blockchain.cpp (1 hunks)
  • src/rpc/mining.cpp (1 hunks)
  • src/rpc/output_script.cpp (1 hunks)
  • src/rpc/rawtransaction_util.cpp (1 hunks)
  • src/rpc/util.cpp (1 hunks)
  • src/script/descriptor.cpp (5 hunks)
  • src/script/miniscript.cpp (5 hunks)
  • src/script/miniscript.h (33 hunks)
  • src/script/script.h (1 hunks)
  • src/test/base58_tests.cpp (2 hunks)
  • src/test/bip32_tests.cpp (1 hunks)
  • src/test/blockfilter_tests.cpp (1 hunks)
  • src/test/descriptor_tests.cpp (2 hunks)
  • src/test/fuzz/miniscript.cpp (1 hunks)
  • src/test/fuzz/miniscript_decode.cpp (0 hunks)
  • src/test/fuzz/txorphan.cpp (0 hunks)
  • src/test/key_io_tests.cpp (3 hunks)
  • src/test/miniscript_tests.cpp (3 hunks)
  • src/test/script_tests.cpp (1 hunks)
  • src/test/sighash_tests.cpp (1 hunks)
  • src/test/transaction_tests.cpp (4 hunks)
  • src/test/util_tests.cpp (1 hunks)
  • src/test/validation_block_tests.cpp (1 hunks)
  • src/univalue/include/univalue.h (1 hunks)
  • src/univalue/lib/univalue.cpp (2 hunks)
  • src/util/message.cpp (0 hunks)
  • src/util/strencodings.cpp (0 hunks)
  • src/util/string.cpp (1 hunks)
  • src/util/string.h (0 hunks)
  • src/util/threadinterrupt.h (1 hunks)
  • src/wallet/bdb.cpp (1 hunks)
  • src/wallet/hdchain.cpp (2 hunks)
  • src/wallet/receive.cpp (1 hunks)
  • src/wallet/scriptpubkeyman.cpp (1 hunks)
  • src/wallet/spend.cpp (1 hunks)
  • src/wallet/walletdb.cpp (2 hunks)
  • test/README.md (1 hunks)
  • test/get_previous_releases.py (3 hunks)
  • test/lint/run-lint-format-strings.py (0 hunks)
💤 Files with no reviewable changes (7)
  • src/util/message.cpp
  • src/util/string.h
  • test/lint/run-lint-format-strings.py
  • src/util/strencodings.cpp
  • src/test/fuzz/txorphan.cpp
  • src/bench/wallet_loading.cpp
  • src/test/fuzz/miniscript_decode.cpp
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

  • src/test/sighash_tests.cpp
  • src/key.cpp
  • src/util/threadinterrupt.h
  • src/wallet/bdb.cpp
  • src/pubkey.h
  • src/test/descriptor_tests.cpp
  • src/blockfilter.cpp
  • src/test/util_tests.cpp
  • src/logging.cpp
  • src/test/fuzz/miniscript.cpp
  • src/wallet/scriptpubkeyman.cpp
  • src/test/transaction_tests.cpp
  • src/key.h
  • src/rpc/util.cpp
  • src/wallet/walletdb.cpp
  • src/script/descriptor.cpp
  • src/node/blockstorage.cpp
  • src/outputtype.cpp
  • src/test/bip32_tests.cpp
  • src/univalue/include/univalue.h
  • src/pubkey.cpp
  • src/test/script_tests.cpp
  • src/test/miniscript_tests.cpp
  • src/wallet/spend.cpp
  • src/test/key_io_tests.cpp
  • src/wallet/hdchain.cpp
  • src/bitcoin-tx.cpp
  • src/outputtype.h
  • src/init.cpp
  • src/wallet/receive.cpp
  • src/test/blockfilter_tests.cpp
  • src/rpc/blockchain.cpp
  • src/rest.cpp
  • src/rpc/output_script.cpp
  • src/univalue/lib/univalue.cpp
  • src/rpc/mining.cpp
  • src/script/miniscript.h
  • src/test/validation_block_tests.cpp
  • src/rpc/rawtransaction_util.cpp
  • src/qt/splashscreen.cpp
  • src/netbase.cpp
  • src/coins.cpp
  • src/script/miniscript.cpp
  • src/core_read.cpp
  • src/util/string.cpp
  • src/test/base58_tests.cpp
  • src/script/script.h
  • src/bitcoin-cli.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

  • src/test/sighash_tests.cpp
  • src/test/descriptor_tests.cpp
  • src/test/util_tests.cpp
  • src/test/fuzz/miniscript.cpp
  • src/test/transaction_tests.cpp
  • src/test/bip32_tests.cpp
  • src/test/script_tests.cpp
  • src/test/miniscript_tests.cpp
  • src/test/key_io_tests.cpp
  • src/test/blockfilter_tests.cpp
  • src/test/validation_block_tests.cpp
  • src/test/base58_tests.cpp
contrib/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)

Files:

  • contrib/guix/libexec/build.sh
ci/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the ci directory (continuous integration)

Files:

  • ci/test/00_setup_env_native_qt5.sh
  • ci/test/05_before_script.sh
  • ci/dash/lint-tidy.sh
src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue

Files:

  • src/univalue/include/univalue.h
  • src/univalue/lib/univalue.cpp
🧠 Learnings (1)
📓 Common learnings 
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🧬 Code graph analysis (12)
src/pubkey.h (2)
src/key.cpp (4)
  • Derive (287-305)
  • Derive (287-287)
  • Derive (343-350)
  • Derive (343-343)
src/pubkey.cpp (4)
  • Derive (240-259)
  • Derive (240-240)
  • Derive (311-318)
  • Derive (311-311)
src/test/fuzz/miniscript.cpp (1)
src/test/miniscript_tests.cpp (8)
  • a (74-76)
  • a (74-74)
  • key (79-79)
  • key (79-79)
  • key (82-87)
  • key (82-82)
  • FromString (91-96)
  • FromString (91-91)
src/key.h (1)
src/key.cpp (4)
  • Derive (287-305)
  • Derive (287-287)
  • Derive (343-350)
  • Derive (343-343)
ci/test/05_before_script.sh (1)
ci/test/04_install.sh (1)
  • CI_EXEC (71-73)
src/univalue/include/univalue.h (1)
src/univalue/lib/univalue.cpp (12)
  • push_back (104-109)
  • push_back (104-104)
  • push_backV (111-116)
  • push_backV (111-111)
  • __pushKV (118-124)
  • __pushKV (118-118)
  • key (187-197)
  • key (187-187)
  • pushKV (126-135)
  • pushKV (126-126)
  • pushKVs (137-143)
  • pushKVs (137-137)
src/test/miniscript_tests.cpp (1)
src/test/fuzz/miniscript.cpp (12)
  • a (50-52)
  • a (50-50)
  • a (97-99)
  • a (97-97)
  • key (54-60)
  • key (54-54)
  • key (101-105)
  • key (101-101)
  • key (107-112)
  • key (107-107)
  • FromString (63-68)
  • FromString (63-63)
src/rpc/blockchain.cpp (3)
src/test/script_tests.cpp (1)
  • script (286-286)
src/wallet/interfaces.cpp (4)
  • script (188-195)
  • script (188-188)
  • script (204-208)
  • script (204-204)
src/script/descriptor.cpp (2)
  • scripts (758-763)
  • scripts (758-758)
src/rest.cpp (1)
src/rpc/mempool.cpp (4)
  • MempoolToJSON (332-371)
  • MempoolToJSON (332-332)
  • MempoolInfoToJSON (682-699)
  • MempoolInfoToJSON (682-682)
src/script/miniscript.h (2)
src/script/miniscript.cpp (12)
  • ComputeType (36-248)
  • ComputeType (36-36)
  • ComputeScriptLen (250-280)
  • ComputeScriptLen (250-250)
  • SanitizeType (16-34)
  • SanitizeType (16-16)
  • FindNextChar (331-339)
  • FindNextChar (331-331)
  • DecomposeScript (282-316)
  • DecomposeScript (282-282)
  • ParseScriptNumber (318-329)
  • ParseScriptNumber (318-318)
src/test/miniscript_tests.cpp (6)
  • a (74-76)
  • a (74-74)
  • key (79-79)
  • key (79-79)
  • key (82-87)
  • key (82-82)
src/test/validation_block_tests.cpp (2)
src/rpc/mining.cpp (2)
  • block (987-992)
  • block (987-987)
src/net_processing.cpp (2)
  • block (608-608)
  • block (612-612)
src/rpc/rawtransaction_util.cpp (1)
src/rpc/request.cpp (2)
  • JSONRPCError (56-62)
  • JSONRPCError (56-56)
src/script/miniscript.cpp (3)
src/script/script.cpp (2)
  • CheckMinimalPush (386-409)
  • CheckMinimalPush (386-386)
src/script/standard.h (1)
  • IsPushdataOp (112-115)
src/script/script.h (2)
  • CScriptNum (211-226)
  • CScriptNum (230-240)
🪛 Flake8 (7.3.0)
test/get_previous_releases.py

[error] 324-324: continuation line over-indented for hanging indent

(E126)

[error] 325-325: continuation line over-indented for hanging indent

(E126)

🪛 GitHub Actions: Clang Diff Format Check
src/wallet/hdchain.cpp

[error] 130-131: Clang-format differences detected in 'src/wallet/hdchain.cpp'. The diff shows a function signature wrapping across lines. Run clang-format-diff.py for formatting and re-run the CI check.

🪛 markdownlint-cli2 (0.18.1)
test/README.md

105-105: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: mac-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (76)
src/node/blockstorage.cpp (1)

451-451: LGTM! Good performance optimization.

Binding blocksdir as a const reference instead of by-value avoids an unnecessary copy of the fs::path object. The reference lifetime is safe since it's only used within the immediate loop scope.

src/rpc/output_script.cpp (1)

38-41: LGTM! Named parameter comments improve readability.

The update from /* optional */ true to /*optional=*/true follows the named parameter comment convention, making it immediately clear that the boolean value corresponds to the optional parameter. This is a standard C++ documentation practice that improves code maintainability.

src/test/util_tests.cpp (1)

1307-1307: LGTM! Appropriate use of NOLINT suppressions.

The three NOLINT comments correctly suppress false-positive use-after-move warnings. This test intentionally accesses the addresses of moved-from objects to verify that the Tracker class preserves origin pointers through move operations, which is well-defined behavior in C++.

Also applies to: 1313-1313, 1321-1321

src/rpc/mining.cpp (1)

758-758: LGTM! Performance optimization by eliminating unnecessary copy.

The change from copying the string to binding a const reference is correct and safe. The lpval object remains in scope throughout the usage of lpstr (lines 760-761), and UniValue::get_str() returns a const reference to its internal string, so the reference remains valid.

src/wallet/spend.cpp (1)

316-320: LGTM! Valid move semantics optimization.

Changing the loop variable from const COutput& to COutput& correctly enables move semantics when inserting coins into the result map. Since AvailableCoinsListUnspent(wallet).all() returns a temporary vector, moving from its elements avoids unnecessary copies while maintaining identical behavior. All member accesses occur before the std::move, and moved-from elements are never used again.

src/outputtype.h (1)

21-21: LGTM - Declaration correctly updated to match implementation.

The extern declaration properly reflects the narrowed OUTPUT_TYPES array size in the corresponding cpp file.

src/outputtype.cpp (1)

21-21: OUTPUT_TYPES reduction from 2 to 1 element is safe—no code iterates this array.

Comprehensive search found only the declaration (header) and definition in src/outputtype.cpp:21. No code in the codebase iterates or depends on OUTPUT_TYPES array size. The removal of OutputType::UNKNOWN is intentional, treating it as a sentinel for internal defensive programming while keeping LEGACY as the only public output type. The enum value itself remains available for switch statements.

src/netbase.cpp (1)

393-393: LGTM! Good refactoring.

These changes correctly simplify the code by removing unnecessary assignments to recvr when only a binary success/failure check is needed. The variable recvr is still properly used at lines 441-442 and 465-479 where the specific error type (e.g., Timeout) needs to be distinguished.

Also applies to: 416-416, 482-482

src/rest.cpp (2)

623-657: LGTM! Clean endpoint consolidation.

The unified rest_mempool handler effectively consolidates the previous separate mempool endpoints into a single function with proper sub-path routing. The logic correctly validates the sub-path parameter and routes to the appropriate helper function (MempoolToJSON for "contents", MempoolInfoToJSON for "info"). The parameter naming change to str_uri_part improves consistency with other handlers in the file.

968-968: LGTM! Dispatch table correctly updated.

The dispatch table entry properly maps the unified endpoint prefix to the new consolidated handler.

src/rpc/blockchain.cpp (1)

2494-2498: LGTM! Clean move-semantics optimization.

Changing the loop variable from const CScript& to CScript& enables the move on line 2497, avoiding one copy when inserting into descriptors. The sequence is correct: script is copied into needles first (line 2496), then moved into descriptors (line 2497). Since scripts is local and not used after this loop, leaving elements in a moved-from state is safe.

src/wallet/hdchain.cpp (1)

156-164: Code changes verified—exception handling is appropriate.

Both call sites properly allow exceptions to propagate to their callers, which is the correct pattern for modern C++ error handling. LegacyScriptPubKeyMan::GetKey (line 1184) and LegacyScriptPubKeyMan::DeriveNewChildKey (line 1308) are exception-aware functions that are equipped to propagate errors upstream.

contrib/guix/libexec/build.sh (1)

360-361: LGTM! rpcauth directory now included in distribution.

The recursive copy correctly packages the rpcauth directory into the distribution tarball, aligning with the Windows installer changes.

share/setup.nsi.in (2)

134-135: LGTM! Uninstaller cleanup correctly mirrors installation.

The uninstaller appropriately removes both the dash.conf file and the share directory (including rpcauth), matching the files installed in the SEC0000 section.

78-80: The original review comment is incorrect.

The share/rpcauth directory contains exactly two files: README.md and rpcauth.py. Both have file extensions. The NSIS *.* wildcard pattern matches files with extensions in the filename portion and will successfully include both files. There are no files without extensions in the directory, so the concern about incomplete packaging is unfounded.

Likely an incorrect or invalid review comment.

src/bitcoin-cli.cpp (1)

951-955: Safe copy-elision; LGTM

Binding wallet_name as const std::string& here is safe: the reference lifetime spans the loop iteration, and both subsequent uses copy the string (into std::optional<std::string> in ConnectAndCallRPC and into balances.pushKV). This also avoids an extra intermediate copy compared to the previous by‑value binding.

src/Makefile.test.include (1)

324-324: LGTM: Fuzz test source replacement.

The replacement of miniscript_decode.cpp with miniscript.cpp aligns with the broader test infrastructure improvements introducing Miniscript roundtrip testing.

src/util/threadinterrupt.h (1)

9-9: LGTM: Explicit dependency declaration.

Adding the threadsafety.h include explicitly declares the dependency for the EXCLUSIVE_LOCKS_REQUIRED annotations used on lines 26 and 28, following the best practice of including headers that define symbols you use directly.

src/blockfilter.cpp (1)

173-173: LGTM: Avoids unnecessary copy.

Iterating with a const reference prevents copying each std::map entry during iteration while preserving the same semantics.

src/test/key_io_tests.cpp (1)

31-31: LGTM: Avoids unnecessary UniValue copies.

Binding test objects as const references across all three test loops eliminates unnecessary copies while maintaining const-correctness, consistent with similar optimizations throughout the test suite.

Also applies to: 89-89, 129-129

src/wallet/receive.cpp (1)

431-431: LGTM: Avoids copying sets.

Iterating with a const reference prevents copying each std::set<CTxDestination> from groupings during iteration, which can be expensive if sets contain multiple addresses.

src/test/validation_block_tests.cpp (1)

185-185: LGTM: Improves const-correctness.

Using a const reference clarifies that block is not modified in this loop and prevents accidental mutation, while still allowing the shared_ptr to be copied when passed to ProcessNewBlock.

src/core_read.cpp (1)

192-192: LGTM: Avoids string copy.

Binding the hash type string as a const reference avoids an unnecessary copy of the string returned by get_str(), while safely extending the lifetime of the temporary for the duration of the function scope.

src/wallet/bdb.cpp (1)

446-446: LGTM: Avoids copying map entries.

Iterating with a const reference prevents copying each entry from m_databases during iteration, improving performance while preserving identical semantics.

src/bitcoin-tx.cpp (1)

558-558: LGTM!

Binding by const reference avoids copying the UniValue object while maintaining read-only access. The lifetime is safe since prevtxsObj remains in scope throughout the loop.

src/wallet/walletdb.cpp (1)

887-887: LGTM!

Using const references in these range-based for loops avoids copying map elements (pairs of descriptor IDs and their associated data). The elements are only read, not modified, making const references the appropriate choice.

Also applies to: 894-894, 904-904

src/rpc/util.cpp (1)

102-102: LGTM!

Binding a const reference to the string returned by v.get_str() eliminates an unnecessary copy. The string is only read (length check, hex validation), and the lifetime is safe since v remains in scope throughout the function.

src/test/script_tests.cpp (1)

884-884: LGTM!

Binding the test variable as a const reference avoids copying the UniValue object in each iteration. The test data is only read, making this optimization safe and appropriate.

src/test/blockfilter_tests.cpp (1)

147-147: LGTM!

Binding the test variable as a const reference avoids copying the UniValue object in each iteration. The test data is only read, making this optimization safe and appropriate.

src/init.cpp (1)

1410-1410: LGTM: Avoid unnecessary copy by binding const reference.

The change from value binding to const reference binding avoids copying the path object. This is safe regardless of whether GetDataDirNet() returns by value (temporary lifetime is extended) or by reference.

src/test/sighash_tests.cpp (1)

164-164: LGTM: Avoid copying test data.

Binding the test element as a const reference instead of copying avoids unnecessary overhead while preserving the immutability of the test data.

src/test/base58_tests.cpp (1)

27-27: LGTM: Consistent const-reference binding in test loops.

Both test functions now bind test elements as const references, avoiding unnecessary copies while maintaining test data immutability.

Also applies to: 49-49

src/coins.cpp (1)

359-361: LGTM: Avoid copying callbacks in error handler.

Binding callback objects as const references avoids potentially expensive copies of std::function objects during iteration.

src/rpc/rawtransaction_util.cpp (1)

149-149: LGTM: Avoid copying UniValue objects.

Both changes eliminate unnecessary copies by binding const references to the results of get_array() and get_obj(). The usage is read-only in both cases, making const references appropriate.

Also applies to: 156-156

src/logging.cpp (1)

465-467: LGTM: Avoid copying thread name.

Binding the thread name as a const reference eliminates an unnecessary copy. The value is only used for read-only string formatting, making this change appropriate.

src/util/string.cpp (1)

10-13: LGTM: Remove invalid move from const reference.

The std::move() on a const reference parameter is invalid and would be ignored by the compiler (resulting in a copy anyway). Removing it makes the code clearer and semantically correct.

ci/dash/lint-tidy.sh (1)

45-45: LGTM: Add modified file to IWYU analysis.

Including src/util/string.cpp in the include-what-you-use checks is appropriate given the recent modifications to that file.

src/wallet/scriptpubkeyman.cpp (1)

1929-1929: LGTM: Performance improvement by avoiding unnecessary copies.

The change from by-value to const-reference iteration eliminates copying of map elements during iteration, which is especially beneficial for maps containing complex value types like std::pair<CPubKey, std::vector<unsigned char>>.

src/qt/splashscreen.cpp (1)

58-58: LGTM: Const-reference binding reduces unnecessary copy.

Binding a const reference to the temporary returned by getTitleAddText() is safe and efficient. The lifetime of the temporary is extended to match the lifetime of the const reference, and all subsequent usages are read-only.

src/.clang-tidy (1)

4-29: LGTM: Enhanced static analysis coverage.

The addition of performance and correctness checks aligns well with the PR's focus on reducing unnecessary copies and improving const-correctness. The CheckTriviallyCopyableMove=false configuration is a reasonable choice to prevent overly aggressive warnings for trivially copyable types.

src/key.h (1)

138-138: LGTM: [[nodiscard]] enforces proper error handling.

Adding [[nodiscard]] to the Derive methods ensures callers properly check for derivation failures, which is critical for key derivation correctness. This change aligns with the error-handling improvements throughout the PR, particularly in src/script/descriptor.cpp where Derive calls are now properly checked.

Also applies to: 189-189

src/script/descriptor.cpp (3)

307-312: LGTM: Proper error handling for key derivation.

Replacing unconditional derivation with checked calls that return false on failure is a significant robustness improvement. This change properly propagates derivation errors instead of silently continuing with invalid state, and aligns with the [[nodiscard]] attributes added to the Derive methods in src/key.h.

Also applies to: 367-373

476-477: LGTM: Consistent error handling for private key derivation.

The guarded derivation calls in GetPrivKey ensure that derivation failures are properly detected and propagated, maintaining consistency with the error-handling improvements throughout this file.

550-550: LGTM: Correct removal of unnecessary move.

Removing std::move from the string concatenation is appropriate. The compiler already optimizes concatenation with temporaries, and explicitly moving here provides no benefit while potentially reducing readability.

Also applies to: 574-574

src/script/script.h (1)

556-569: No issues found—this is an intentional optimization.

The git history shows commit f5d476aec1 Merge bitcoin/bitcoin#25709: script: actually trigger the optimization in BuildScript, confirming this refactoring was intentionally merged from Bitcoin upstream as an optimization.

The behavioral change is semantically equivalent: assigning the first CScript to an empty ret and then appending subsequent ones produces the same result as appending all inputs. The optimization avoids unnecessary vector operations for the first element. Additionally, operator<< is explicitly deleted for CScript types, preventing accidental misuse. All callers in miniscript.h and miniscript.cpp are compatible with this behavior.

src/test/transaction_tests.cpp (4)

156-156: LGTM: Performance optimization.

The change from value to const reference eliminates unnecessary copies of UniValue objects during iteration.

175-175: LGTM: Consistent optimization.

Same performance improvement as line 156, avoiding unnecessary copies.

240-240: LGTM: Consistent pattern.

Continues the const-reference optimization pattern for test iteration.

259-259: LGTM: Final optimization.

Completes the const-reference pattern for nested loop iteration.

src/pubkey.cpp (1)

312-312: LGTM: Overflow prevention guard.

The depth-limit guard correctly prevents overflow by failing derivation when nDepth is already at the maximum value (255) for unsigned char, before attempting to increment to 256 at line 313.

src/test/descriptor_tests.cpp (2)

183-183: LGTM: Validates Derive success.

Wrapping the Derive call with BOOST_CHECK ensures the derivation succeeds and aligns with the [[nodiscard]] attribute on the Derive method.

206-206: LGTM: Consistent validation.

Same pattern as line 183, ensuring Derive calls are checked for success.

src/key.cpp (1)

344-344: LGTM: Consistent overflow guard.

This depth-limit guard mirrors the implementation in CExtPubKey::Derive (src/pubkey.cpp line 312), ensuring consistent behavior between private and public key derivation paths.

src/test/bip32_tests.cpp (1)

187-203: LGTM: Comprehensive depth-limit test.

This new test case properly validates the depth-limit guards added in CExtKey::Derive and CExtPubKey::Derive. It confirms that:

  • Derivation succeeds up to depth 255
  • Derivation fails when attempting to derive at depth 256
  • Both private and public key paths behave consistently
ci/test/05_before_script.sh (1)

33-34: Code changes verified as correct.

Environment variable DOWNLOAD_PREVIOUS_RELEASES is properly set in ci/test/00_setup_env_native_qt5.sh:17. The script test/get_previous_releases.py correctly handles being called without explicit tags arguments, using internal defaults from the SHA256_SUMS dictionary when no tags are specified.

test/get_previous_releases.py (1)

23-105: SHA256_SUMS struct change LGTM.

Mapping hash → {tag, tarball} is clearer and enables tag defaults. Proceeding with downstream fixes below.

src/test/fuzz/miniscript.cpp (1)

135-139: Fuzz init looks good.

ECC_Start + precomputed data is appropriate for roundtrips.

src/test/miniscript_tests.cpp (2)

74-113: KeyConverter API updates align with optional-return miniscript.

Clean switch to std::optional across From* helpers and added comparator.

282-294: Good duplicate-key sanity coverage.

These cases improve IsSane/CheckDuplicateKey checks without altering behavior.

src/pubkey.h (2)

211-211: [[nodiscard]] on CPubKey::Derive — audit complete.

All call sites properly check the bool return value. Verified across:

  • Descriptors (line 373 checks der after assignments)
  • Wallet operations (checked with throw on failure)
  • Tests (BOOST_CHECK and assert patterns)
  • Internal propagation (CExtPubKey::Derive returns result from CPubKey::Derive)

The [[nodiscard]] attribute is well-placed and callers consistently validate failures.

284-284: CExtPubKey::Derive is properly annotated and all call sites handle the return value.

Verification confirms: the [[nodiscard]] attribute is already present on the declaration (src/pubkey.h:284), and all 7 call sites throughout the codebase properly handle the return value via BOOST_CHECK assertions, conditional statements, or variable assignment. No dropped-result calls exist.

src/script/miniscript.cpp (4)

20-33: LGTM: Assertion formatting improvements.

The assertion changes improve readability without altering logic.

36-248: LGTM: Systematic refactoring to fragment-centric API.

The parameter rename from nodetype to fragment is applied consistently throughout the function, including sanity checks and the switch statement. The logic remains unchanged.

250-280: LGTM: Consistent fragment parameter usage.

The refactoring is consistent with the changes in ComputeType.

282-329: LGTM: Modern optional-based error handling.

The refactoring from bool return + output parameter to std::optional<T> return is a good modernization that makes error handling more explicit and eliminates side-effects through output parameters.

src/script/miniscript.h (11)

9-9: LGTM: Public Opcode alias improves API clarity.

Exposing Opcode as a public type alias makes the API clearer and more maintainable.

Also applies to: 183-183

230-230: LGTM: Function signatures updated consistently.

The signatures match the implementation changes in miniscript.cpp.

Also applies to: 233-233

285-285: LGTM: Added duplicate key tracking.

Caching the duplicate_key result during construction is good for performance, as the check involves a tree traversal.

Also applies to: 304-305

337-415: LGTM: Convenient TreeEvalMaybe overload added.

The simpler overload without downfn is a useful convenience method. The implementation correctly wraps the full version with a dummy state.

432-450: LGTM: Non-recursive comparison prevents stack overflow.

The iterative comparison algorithm using an explicit queue is a good safeguard against stack overflow on deeply nested miniscript trees.

540-639: LGTM: Proper optional error propagation in ToString.

The method correctly propagates errors from ctx.ToString() by checking each optional result and returning {} on failure.

769-799: LGTM: Duplicate key detection with good optimization.

The implementation correctly detects duplicates both within nodes and across subtrees. The optimization to merge smaller sets into larger ones (lines 789-790) improves performance.

830-843: LGTM: Well-structured validation methods.

The layered validation methods provide clear, composable checks. IsSaneSubexpression appropriately combines multiple validation aspects.

849-854: LGTM: Context threading enables duplicate key detection.

All constructors consistently take a context parameter and call ContainsDuplicateKey(ctx) to cache the result, which is good for performance.

1368-1391: LGTM: Proper optional handling and context threading.

The rest of DecodeScript correctly handles optional returns from context methods and consistently passes the context to all MakeNodeRef calls.

Also applies to: 1404-1686

1705-1714: LGTM: Proper optional handling in FromScript.

The function correctly checks the optional result from DecomposeScript and properly dereferences it to get iterators.

src/univalue/include/univalue.h Outdated
Comment on lines 81 to 88
void push_back(UniValue val);
void push_backV(const std::vector<UniValue>& vec);
template <class It>
void push_backV(It first, It last);

void __pushKV(const std::string& key, const UniValue& val);
void pushKV(const std::string& key, const UniValue& val);
void pushKVs(const UniValue& obj);
void __pushKV(std::string key, UniValue val);
void pushKV(std::string key, UniValue val);
void pushKVs(UniValue obj);
Copy link

@coderabbitai coderabbitai bot Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Do not modify vendored dependencies.

These header changes to the vendored UniValue library violate the coding guidelines. The univalue library is a vendored dependency that must not be modified directly. Any API changes should be contributed upstream first, then the updated version should be pulled in through the standard vendoring process.

Based on coding guidelines.

🤖 Prompt for AI Agents 
In src/univalue/include/univalue.h lines 81-88 the header was altered to
add/rename push_backV/pushKV-style APIs in a vendored UniValue library, which
violates the "do not modify vendored dependencies" policy; revert these changes
in the vendored file to match the upstream version, remove any references to the
new/renamed symbols from project code, and instead implement any required
adapters in your own source (either a thin wrapper utility that translates calls
to the upstream UniValue API or new helper functions in the project tree), or
submit a PR to the UniValue project and update the vendored copy only after the
upstream change is accepted and pulled in through the normal vendoring process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@coderabbitai: learn 2 things:

  1. backported changes should be matching with original PR's from bitcoin/bitcoin, bitcoin-core/gui, etc; even if they looks strange, or changes vendored code, or violating guidelines. Still need to warn about bugs, UB, crashes, compilation errors, linters, etc.
  2. univalue is no more vendored, but part of bitcoin's codebase so far it is not supported anymore.

Copy link

Choose a reason for hiding this comment

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

@knst, thank you for the clarification! I understand now:

  1. Backport fidelity: For backported changes from bitcoin/bitcoin or bitcoin-core/gui, I should ensure they match the original upstream PRs, even if they appear unusual or would normally violate guidelines. I'll still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter problems.

  2. UniValue status: UniValue is no longer a vendored external dependency but is now maintained as part of the Bitcoin codebase itself, so modifications to it are acceptable as part of backports.

I notice there's an existing learning (ID: 8daec6cf-7f8e-439b-9077-4f41b0a4661a) that lists src/univalue among the vendored dependencies that shouldn't be modified. Would you like me to remove that learning or update it to exclude univalue from the list of protected vendored directories?

✏️ Learnings added 
Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

greptile-apps[bot]
greptile-apps bot reviewed Oct 25, 2025
View reviewed changes
Copy link

@greptile-apps greptile-apps bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR backports 14 Bitcoin Core PRs from v24 to Dash Core, focusing on code modernization, performance optimizations, and API safety improvements. The changes include adopting std::optional for error handling, eliminating unnecessary object copies throughout the codebase, adding [[nodiscard]] attributes to critical cryptographic functions, improving miniscript validation with duplicate key detection, and enhancing header include hygiene. Most changes are mechanical optimizations that avoid copying objects (particularly UniValue, strings, and path objects) by using const references in range-based loops. The PR also adds safety checks to BIP32 key derivation to prevent overflow at maximum depth and improves error handling in descriptor parsing. The changes integrate cleanly with Dash Core's architecture, touching wallet code, RPC handlers, test infrastructure, and build system configurations.

PR Description Notes:

  • The PR description indicates testing is incomplete with "WAITING_CI_FOR_LINTERS TODO" status

Important Files Changed

Filename Score Overview
src/util/string.h 1/5 Removed <algorithm> include but code still uses std::equal which requires it - will likely cause compilation failure
test/get_previous_releases.py 1/5 Type inconsistency bug where tags default to set but user input is list; tarball validation logic broken
src/util/message.cpp 2/5 Removed <serialize.h> include but code uses CHashWriter from that header - may fail unless included transitively
src/outputtype.h 4/5 Reduced OUTPUT_TYPES array size from2 to 1 (only LEGACY), potentially breaking code expecting multiple types
src/outputtype.cpp 4/5 Removed UNKNOWN from OUTPUT_TYPES array while keeping it in enum - creates inconsistency in iteration contexts
src/script/miniscript.h 4/5 Breaking API changes requiring Ctx parameter in constructors and changing return types to std::optional
src/univalue/lib/univalue.cpp 4/5 Potential evaluation order issue in pushKV where key is used by reference before being moved
ci/test/00_setup_env_native_qt5.sh 4/5 Changed from explicit version list to boolean flag for downloading releases - requires CI config consistency
ci/test/05_before_script.sh 4/5 Refactored previous release download logic to use boolean flag instead of version list
test/lint/run-lint-format-strings.py 4/5 Removed false positives for LogPrintf templates - may expose previously ignored format string issues
src/rest.cpp 4/5 Consolidated mempool endpoints but URI validation doesn't verify format extension exists
test/README.md 5/5 Simplified backwards compatibility testing instructions by removing version list
src/core_read.cpp 5/5 Performance optimization using const reference to avoid string copy
src/test/fuzz/miniscript_decode.cpp 5/5 File deleted as fuzzing functionality consolidated into miniscript.cpp
src/logging.cpp 5/5 Performance optimization using const reference for thread name
src/test/sighash_tests.cpp 5/5 Performance optimization using const reference in test loop
src/blockfilter.cpp 5/5 Performance optimization using const reference when iterating filter types
src/wallet/receive.cpp 5/5 Performance optimization passing address groupings by const reference
src/test/script_tests.cpp 5/5 Performance optimization using const reference in script test loop
contrib/guix/libexec/build.sh 5/5 Added share/rpcauth directory to binary distribution
src/test/key_io_tests.cpp 5/5 Performance optimizations using const references in three test loops
src/pubkey.cpp 5/5 Added overflow protection to prevent derivation beyond maximum depth 255
ci/dash/lint-tidy.sh 5/5 Added src/util/string.cpp to IWYU linter configuration
src/bitcoin-tx.cpp 5/5 Performance optimization using const reference in transaction signing loop
src/util/strencodings.cpp 5/5 Removed unused algorithm header include
src/node/blockstorage.cpp 5/5 Performance optimization storing blocks directory path as const reference
src/wallet/scriptpubkeyman.cpp 5/5 Performance optimization using const reference when iterating crypted keys
src/bitcoin-cli.cpp 5/5 Performance optimization using const reference for wallet names
src/test/blockfilter_tests.cpp 5/5 Performance optimization using const reference in blockfilter test loop
src/test/fuzz/miniscript.cpp 5/5 New comprehensive fuzzing test file for miniscript functionality
src/test/validation_block_tests.cpp 5/5 Performance optimization using const reference when iterating blocks
src/test/base58_tests.cpp 5/5 Performance optimizations using const references in two test loops
src/Makefile.test.include 5/5 Updated build system to reference renamed miniscript fuzz test file
src/key.cpp 5/5 Added overflow protection to CExtKey derivation at maximum depth
src/wallet/bdb.cpp 5/5 Performance optimization using const reference when iterating databases
src/wallet/hdchain.cpp 5/5 Added DeriveExtKey helper function that throws exceptions on derivation failure
src/script/script.h 5/5 Fixed counter increment timing in BuildScript to correctly identify first element
src/key.h 5/5 Added [[nodiscard]] attributes to BIP32 key derivation methods
src/rpc/output_script.cpp 5/5 Standardized comment syntax and removed unused NodeContext declaration
src/pubkey.h 5/5 Added [[nodiscard]] attributes to CPubKey and CExtPubKey Derive methods
src/rpc/blockchain.cpp 5/5 Enabled move semantics for CScript objects in scantxoutset loop
src/util/string.cpp 5/5 Removed unnecessary std::move on const reference parameter
src/rpc/mining.cpp 5/5 Performance optimization using const reference for longpoll ID string
src/bench/wallet_loading.cpp 5/5 Removed unused imports from wallet benchmark code
src/univalue/include/univalue.h 5/5 Changed UniValue API to use pass-by-value for better move semantics
src/init.cpp 5/5 Performance optimization using const reference for data directory path
src/test/descriptor_tests.cpp 5/5 Added error checking for BIP32 extended public key derivation in tests
src/.clang-tidy 5/5 Added five new clang-tidy checks promoted to errors for code quality
src/coins.cpp 5/5 Performance optimization using const reference when iterating error callbacks
src/test/transaction_tests.cpp 5/5 Performance optimizations using const references in four test loop variables
src/qt/splashscreen.cpp 5/5 Performance optimization using const reference for title text
src/test/fuzz/txorphan.cpp 5/5 Removed unused algorithm header include
src/test/miniscript_tests.cpp 5/5 Modernized KeyConverter interface and added comprehensive duplicate key tests
src/netbase.cpp 5/5 Removed three dead assignments to recvr variable in Socks5 function
share/setup.nsi.in 5/5 Added dash.conf and rpcauth utilities to Windows NSIS installer
src/test/util_tests.cpp 5/5 Added NOLINT comments to suppress use-after-move warnings in intentional test code
src/rpc/util.cpp 5/5 Performance optimization using const reference when parsing hash values
src/script/descriptor.cpp 5/5 Added error handling for BIP32 derivation and removed redundant std::move calls
src/rpc/rawtransaction_util.cpp 5/5 Performance optimizations using const references for UniValue objects
src/util/threadinterrupt.h 5/5 Added thread safety annotations with EXCLUSIVE_LOCKS_REQUIRED
src/wallet/walletdb.cpp 5/5 Performance optimizations using const references in three descriptor-related loops
src/script/miniscript.cpp 5/5 Refactored miniscript with modernized API returning std::optional and improved validation
src/wallet/spend.cpp 5/5 Enabled move semantics for COutput objects in ListCoins loop
src/test/bip32_tests.cpp 5/5 Added new test case verifying BIP32 key derivation depth limits

Confidence score: 2/5

  • This PR has multiple critical compilation failures that will prevent it from building successfully
  • Three files (src/util/string.h, src/util/message.cpp, test/get_previous_releases.py) contain clear bugs that will cause build or runtime failures
  • Files with removed includes (string.h removing algorithm, message.cpp removing serialize.h) still depend on symbols from those headers
  • The get_previous_releases.py script has type inconsistency bugs and broken validation logic that will cause CI failures
  • The miniscript API changes are breaking and require careful verification that all callsites have been updated
  • Multiple files flagged with scores of 4/5 or lower indicate non-trivial integration risks that need attention before merge

Additional Comments (1)

  1. src/test/fuzz/txorphan.cpp, line 29 (link)

    logic: The calculation can overflow when DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE * 1000000 exceeds uint32_t max. Consider using uint64_t for intermediate calculation or adding overflow checks.

Context used:

  • Context from dashboard - CLAUDE.md (source)

64 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

src/univalue/lib/univalue.cpp
if (typ != VOBJ) throw std::runtime_error{"JSON value is not an object as expected"};

size_t idx;
if (findKey(key, idx))
Copy link

Choose a reason for hiding this comment

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

logic: findKey takes key by const reference, so passing a moved-from key to findKey on line 131 after it's moved on line 134 is problematic. The lookup happens before the move, but this relies on order of evaluation. Should we capture the findKey result before moving the key, or does the current ordering guarantee safety?

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/univalue/lib/univalue.cpp
Line: 131:131

Comment:
**logic:** `findKey` takes `key` by const reference, so passing a moved-from `key` to `findKey` on line 131 after it's moved on line 134 is problematic. The lookup happens before the move, but this relies on order of evaluation. Should we capture the findKey result before moving the key, or does the current ordering guarantee safety?

How can I resolve this? If you propose a fix, please make it concise.

src/test/bip32_tests.cpp
CExtPubKey pubkey_parent{DecodeExtPubKey(test1.vDerive[0].pub)}, pubkey_child;

// We can derive up to the 255th depth..
for (auto i = 0; i++ < 255;) {
Copy link

Choose a reason for hiding this comment

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

style: unusual loop syntax with post-increment in condition; for (int i = 0; i < 255; i++) would be clearer

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/bip32_tests.cpp
Line: 192:192

Comment:
**style:** unusual loop syntax with post-increment in condition; `for (int i = 0; i < 255; i++)` would be clearer

How can I resolve this? If you propose a fix, please make it concise.

src/rest.cpp
Comment on lines +630 to 632
if (param != "contents" && param != "info") {
return RESTERR(req, HTTP_BAD_REQUEST, "Invalid URI format. Expected /rest/mempool/<info|contents>.json");
}
Copy link

Choose a reason for hiding this comment

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

logic: URI validation logic should verify format includes the .json extension, not just the path component. Current check allows /rest/mempool/info without format suffix, which ParseDataFormat would return as RESTResponseFormat::UNDEF. Should this endpoint require an explicit format suffix like .json, or is the format suffix optional (defaulting to UNDEF)?

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/rest.cpp
Line: 630:632

Comment:
**logic:** URI validation logic should verify format includes the `.json` extension, not just the path component. Current check allows `/rest/mempool/info` without format suffix, which ParseDataFormat would return as RESTResponseFormat::UNDEF. Should this endpoint require an explicit format suffix like `.json`, or is the format suffix optional (defaulting to UNDEF)?

How can I resolve this? If you propose a fix, please make it concise.

test/get_previous_releases.py Outdated
Comment on lines 170 to 171
if tarballHash not in SHA256_SUMS or SHA256_SUMS[tarballHash]['tarball'] != tarball:
if tarball in SHA256_SUMS.values():
Copy link

Choose a reason for hiding this comment

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

logic: Logic issue: line 171 will always be False. SHA256_SUMS.values() now returns dict objects (e.g., {'tag': 'v21.1.1', 'tarball': '...'}) not strings, so tarball in SHA256_SUMS.values() can never match

Prompt To Fix With AI 
This is a comment left during a code review.
Path: test/get_previous_releases.py
Line: 170:171

Comment:
**logic:** Logic issue: line 171 will always be False. SHA256_SUMS.values() now returns dict objects (e.g., `{'tag': 'v21.1.1', 'tarball': '...'}`) not strings, so `tarball in SHA256_SUMS.values()` can never match

How can I resolve this? If you propose a fix, please make it concise.

coderabbitai[bot]
coderabbitai bot reviewed Oct 25, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/wallet/hdchain.cpp (1)

132-138: Fix clang-format issue before merge.

The CI pipeline reports a clang-format difference on the function signature. Please run clang-format on this file to ensure it matches the project's formatting standards.

Run the following to apply the correct formatting:

#!/bin/bash
# Apply clang-format to fix the formatting issue
clang-format -i src/wallet/hdchain.cpp
🧹 Nitpick comments (2)
test/README.md (1)

103-109: Documentation update looks good.

The updated instructions clearly reflect the new simplified usage pattern with the -b flag. The added explanatory text on line 109 helps users understand the purpose of the command.

For improved syntax highlighting and rendering, consider adding a language identifier to the code block:



+shell

test/get_previous_releases.py -b




Based on static analysis.





src/init.cpp (1)


1408-1414: LGTM: no behavior change; minor copy elision


The const-ref bind is safe within this function and matches upstream style. Optional: prefer value for clarity if upstream parity isn’t required.







📜 Review details

Configuration used: CodeRabbit UI


Review profile: CHILL


Plan: Pro



📥 Commits

Reviewing files that changed from the base of the PR and between 797e264 and dc5144b.





📒 Files selected for processing (60)

    

  • ci/dash/lint-tidy.sh (1 hunks)
    • 

  • ci/dash/test_integrationtests.sh (1 hunks)
    • 

  • ci/test/00_setup_env_native_qt5.sh (1 hunks)
    • 

  • ci/test/05_before_script.sh (1 hunks)
    • 

  • src/.clang-tidy (1 hunks)
    • 

  • src/bench/wallet_loading.cpp (0 hunks)
    • 

  • src/bitcoin-cli.cpp (1 hunks)
    • 

  • src/bitcoin-tx.cpp (1 hunks)
    • 

  • src/blockfilter.cpp (1 hunks)
    • 

  • src/coins.cpp (1 hunks)
    • 

  • src/core_read.cpp (1 hunks)
    • 

  • src/init.cpp (1 hunks)
    • 

  • src/key.cpp (1 hunks)
    • 

  • src/key.h (2 hunks)
    • 

  • src/logging.cpp (1 hunks)
    • 

  • src/netbase.cpp (3 hunks)
    • 

  • src/node/blockstorage.cpp (1 hunks)
    • 

  • src/outputtype.cpp (1 hunks)
    • 

  • src/outputtype.h (1 hunks)
    • 

  • src/pubkey.cpp (1 hunks)
    • 

  • src/pubkey.h (2 hunks)
    • 

  • src/qt/splashscreen.cpp (1 hunks)
    • 

  • src/qt/transactionrecord.cpp (0 hunks)
    • 

  • src/rest.cpp (2 hunks)
    • 

  • src/rpc/blockchain.cpp (1 hunks)
    • 

  • src/rpc/evo.cpp (0 hunks)
    • 

  • src/rpc/masternode.cpp (0 hunks)
    • 

  • src/rpc/mining.cpp (1 hunks)
    • 

  • src/rpc/output_script.cpp (1 hunks)
    • 

  • src/rpc/rawtransaction_util.cpp (1 hunks)
    • 

  • src/rpc/util.cpp (1 hunks)
    • 

  • src/script/descriptor.cpp (5 hunks)
    • 

  • src/test/base58_tests.cpp (2 hunks)
    • 

  • src/test/bip32_tests.cpp (1 hunks)
    • 

  • src/test/blockfilter_tests.cpp (1 hunks)
    • 

  • src/test/descriptor_tests.cpp (2 hunks)
    • 

  • src/test/fuzz/txorphan.cpp (0 hunks)
    • 

  • src/test/key_io_tests.cpp (3 hunks)
    • 

  • src/test/miniscript_tests.cpp (3 hunks)
    • 

  • src/test/script_tests.cpp (1 hunks)
    • 

  • src/test/sighash_tests.cpp (1 hunks)
    • 

  • src/test/transaction_tests.cpp (4 hunks)
    • 

  • src/test/util_tests.cpp (1 hunks)
    • 

  • src/test/validation_block_tests.cpp (1 hunks)
    • 

  • src/univalue/include/univalue.h (1 hunks)
    • 

  • src/univalue/lib/univalue.cpp (2 hunks)
    • 

  • src/util/message.cpp (0 hunks)
    • 

  • src/util/strencodings.cpp (0 hunks)
    • 

  • src/util/string.cpp (1 hunks)
    • 

  • src/util/string.h (0 hunks)
    • 

  • src/util/threadinterrupt.h (1 hunks)
    • 

  • src/wallet/bdb.cpp (1 hunks)
    • 

  • src/wallet/hdchain.cpp (2 hunks)
    • 

  • src/wallet/receive.cpp (1 hunks)
    • 

  • src/wallet/scriptpubkeyman.cpp (1 hunks)
    • 

  • src/wallet/spend.cpp (1 hunks)
    • 

  • src/wallet/walletdb.cpp (2 hunks)
    • 

  • test/README.md (1 hunks)
    • 

  • test/get_previous_releases.py (3 hunks)
    • 

  • test/lint/run-lint-format-strings.py (0 hunks)
    • 






💤 Files with no reviewable changes (9)

    

  • src/bench/wallet_loading.cpp
    • 

  • src/rpc/masternode.cpp
    • 

  • src/test/fuzz/txorphan.cpp
    • 

  • src/rpc/evo.cpp
    • 

  • src/util/strencodings.cpp
    • 

  • src/qt/transactionrecord.cpp
    • 

  • src/util/string.h
    • 

  • test/lint/run-lint-format-strings.py
    • 

  • src/util/message.cpp
    • 






🚧 Files skipped from review as they are similar to previous changes (28)

    

  • ci/test/05_before_script.sh
    • 

  • src/wallet/bdb.cpp
    • 

  • src/test/util_tests.cpp
    • 

  • src/.clang-tidy
    • 

  • src/coins.cpp
    • 

  • src/test/bip32_tests.cpp
    • 

  • src/univalue/include/univalue.h
    • 

  • src/qt/splashscreen.cpp
    • 

  • src/key.cpp
    • 

  • src/wallet/walletdb.cpp
    • 

  • src/pubkey.h
    • 

  • src/wallet/scriptpubkeyman.cpp
    • 

  • src/blockfilter.cpp
    • 

  • src/test/descriptor_tests.cpp
    • 

  • ci/dash/lint-tidy.sh
    • 

  • src/test/script_tests.cpp
    • 

  • src/wallet/spend.cpp
    • 

  • src/test/validation_block_tests.cpp
    • 

  • src/rpc/output_script.cpp
    • 

  • src/pubkey.cpp
    • 

  • src/logging.cpp
    • 

  • src/test/blockfilter_tests.cpp
    • 

  • src/test/sighash_tests.cpp
    • 

  • src/util/string.cpp
    • 

  • src/outputtype.h
    • 

  • src/rpc/util.cpp
    • 

  • src/core_read.cpp
    • 

  • src/test/key_io_tests.cpp
    • 






🧰 Additional context used


📓 Path-based instructions (4)


ci/**

📄 CodeRabbit inference engine (CLAUDE.md)




Unless specifically prompted, avoid making changes to the ci directory (continuous integration)




Files:


    

  • ci/dash/test_integrationtests.sh
    • 

  • ci/test/00_setup_env_native_qt5.sh
    • 






src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1

Dash uses unordered_lru_cache for efficient caching with LRU eviction




Files:


    

  • src/rpc/mining.cpp
    • 

  • src/rpc/rawtransaction_util.cpp
    • 

  • src/key.h
    • 

  • src/wallet/hdchain.cpp
    • 

  • src/wallet/receive.cpp
    • 

  • src/util/threadinterrupt.h
    • 

  • src/init.cpp
    • 

  • src/test/base58_tests.cpp
    • 

  • src/outputtype.cpp
    • 

  • src/bitcoin-tx.cpp
    • 

  • src/test/transaction_tests.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 

  • src/script/descriptor.cpp
    • 

  • src/rpc/blockchain.cpp
    • 

  • src/netbase.cpp
    • 

  • src/bitcoin-cli.cpp
    • 

  • src/univalue/lib/univalue.cpp
    • 

  • src/node/blockstorage.cpp
    • 

  • src/rest.cpp
    • 






src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests




Files:


    

  • src/test/base58_tests.cpp
    • 

  • src/test/transaction_tests.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 






src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/**

📄 CodeRabbit inference engine (CLAUDE.md)




Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue




Files:


    

  • src/univalue/lib/univalue.cpp
    • 






🧠 Learnings (1)


📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.



Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.






🧬 Code graph analysis (6)


src/rpc/rawtransaction_util.cpp (1)



src/rpc/request.cpp (2)

    

  • JSONRPCError (56-62)
    • 

  • JSONRPCError (56-56)
    • 








src/key.h (2)



src/key.cpp (4)

    

  • Derive (287-305)
    • 

  • Derive (287-287)
    • 

  • Derive (343-350)
    • 

  • Derive (343-343)
    • 






src/pubkey.cpp (4)

    

  • Derive (240-259)
    • 

  • Derive (240-240)
    • 

  • Derive (311-318)
    • 

  • Derive (311-311)
    • 








src/wallet/hdchain.cpp (2)



src/wallet/hdchain.h (1)

    

  • nAccountIndex (131-131)
    • 






src/wallet/wallet.h (1)

    

  • fInternal (188-211)
    • 








src/test/miniscript_tests.cpp (1)



src/test/fuzz/miniscript.cpp (12)

    

  • a (50-52)
    • 

  • a (50-50)
    • 

  • a (97-99)
    • 

  • a (97-97)
    • 

  • key (54-60)
    • 

  • key (54-54)
    • 

  • key (101-105)
    • 

  • key (101-101)
    • 

  • key (107-112)
    • 

  • key (107-107)
    • 

  • FromString (63-68)
    • 

  • FromString (63-63)
    • 








src/rpc/blockchain.cpp (3)



src/test/script_tests.cpp (1)

    

  • script (286-286)
    • 






src/wallet/interfaces.cpp (4)

    

  • script (188-195)
    • 

  • script (188-188)
    • 

  • script (204-208)
    • 

  • script (204-204)
    • 






src/script/descriptor.cpp (2)

    

  • scripts (758-763)
    • 

  • scripts (758-758)
    • 








src/rest.cpp (1)



src/rpc/mempool.cpp (4)

    

  • MempoolToJSON (332-371)
    • 

  • MempoolToJSON (332-332)
    • 

  • MempoolInfoToJSON (682-699)
    • 

  • MempoolInfoToJSON (682-682)
    • 








🪛 markdownlint-cli2 (0.18.1)


test/README.md

105-105: Fenced code blocks should have a language specified


(MD040, fenced-code-language)









⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

    

  • GitHub Check: linux64_tsan-build / Build source
    • 

  • GitHub Check: linux64-build / Build source
    • 

  • GitHub Check: linux64_ubsan-build / Build source
    • 

  • GitHub Check: linux64_sqlite-build / Build source
    • 

  • GitHub Check: linux64_fuzz-build / Build source
    • 

  • GitHub Check: mac-build / Build source
    • 

  • GitHub Check: arm-linux-build / Build source
    • 

  • GitHub Check: win64-build / Build source
    • 

  • GitHub Check: Lint / Run linters
    • 






🔇 Additional comments (38)



src/util/threadinterrupt.h (1)


9-9: LGTM! Explicit dependency for thread safety annotations.


The include is necessary for the EXCLUSIVE_LOCKS_REQUIRED annotations used on lines 26 and 28. This makes the dependency explicit rather than relying on transitive includes.





src/bitcoin-tx.cpp (1)


558-558: LGTM! Efficient binding by const reference.


The change from by-value to by-const-reference avoids unnecessary copying of the UniValue object while maintaining the same behavior.





src/rpc/rawtransaction_util.cpp (2)


149-149: LGTM: Efficient const reference binding.


The change from value copy to const reference eliminates an unnecessary UniValue copy while preserving semantics.




156-156: LGTM: Efficient const reference binding.


The change from value copy to const reference eliminates an unnecessary UniValue copy while preserving semantics.





src/netbase.cpp (3)


393-393: LGTM: Simplified error checking


The change eliminates unnecessary intermediate storage when the result is only checked once. The behavior remains identical.




416-416: LGTM: Consistent with the refactoring pattern


Same improvement as line 393 - the result is only checked once, so direct comparison is cleaner.




482-482: LGTM: Final port read simplified


Correctly refactored to direct comparison. Note that line 441 appropriately still stores the result in recvr because the specific error type (Timeout vs other errors) is checked at lines 442-449.





src/rpc/blockchain.cpp (1)


2494-2498: LGTM! Move semantics optimization is correct.


The change from a const reference to a non-const reference enables move semantics when inserting into the descriptors map at line 2497. The operation sequence is sound:


    

  1. InferDescriptor reads the script (line 2495)
    2. 

  2. Script is copied into needles (line 2496)
    3. 

  3. Script is moved into descriptors (line 2497)
    4. 



This optimization reduces overhead by replacing one copy with a move operation, while correctly maintaining the necessary copy into needles since both containers require ownership of the CScript objects.





src/univalue/lib/univalue.cpp (4)


104-109: LGTM: Proper move semantics optimization.


The change from const reference to by-value parameter with std::move is a standard optimization pattern that allows callers to move temporary values directly into storage.




118-124: LGTM: Proper move semantics for key-value insertion.


Both parameters correctly taken by value and moved into storage, enabling efficient insertion of temporary values.




126-135: LGTM: Order of evaluation is safe.


Regarding the past review comment: The order of evaluation is correct and safe. findKey(key, idx) on line 131 is evaluated before any move operations. The move of key only occurs in the else branch (line 134), which executes only when the key is not found. There is no use-after-move issue here.


The implementation correctly:


    

  • Updates existing keys by moving only the value (line 132)
    • 

  • Inserts new keys by moving both key and value (line 134)
    • 





137-143: LGTM: Proper move semantics for bulk insertion.


Taking obj by value and moving from it during iteration is the correct pattern for "sink" functions that consume their argument.





src/test/base58_tests.cpp (1)


27-27: LGTM! Good const-correctness improvement.


Binding test data by const reference eliminates unnecessary UniValue copies in the loop without altering test behavior.


Also applies to: 49-49





src/wallet/receive.cpp (1)


431-431: LGTM! Eliminates unnecessary set copy.


Iterating by const reference avoids copying the std::set<CTxDestination> on each iteration while preserving correct behavior.





src/test/transaction_tests.cpp (1)


156-156: LGTM! Const-correctness improvements across test cases.


All four changes appropriately bind UniValue objects by const reference, eliminating unnecessary copies in both tx_valid and tx_invalid test cases.


Also applies to: 175-175, 240-240, 259-259





src/test/miniscript_tests.cpp (6)


74-76: LGTM! Key comparison method added.


The KeyCompare method provides semantic ordering for keys, consistent with similar implementations in the fuzz test suite.




91-96: LGTM! Modernized to optional return type.


The change from bool return to std::optional<Key> is idiomatic modern C++ that makes the API safer and more expressive.




99-103: LGTM! Consistent optional return pattern.


FromPKBytes follows the same modern optional-based API as FromString, providing consistent error handling.




106-113: LGTM! Completes optional-based API.


FromPKHBytes adopts the optional return type for consistency with other parsing methods, even though it always succeeds in the test environment due to assertions.




119-121: LGTM! Legitimate linter suppression.


The NOLINTNEXTLINE comment correctly suppresses a false positive warning about the unused user-defined literal operator, with appropriate bug reference.




282-294: LGTM! Comprehensive duplicate key detection tests.


The new test cases thoroughly validate that miniscripts with duplicate keys parse successfully but correctly fail both IsSane() and CheckDuplicateKey() checks across various structural scenarios.





src/rest.cpp (3)


630-632: Acknowledge past review comment: URI validation behavior is acceptable for backport.


The past review comment correctly observes that the validation at lines 630-632 checks the path component (param) but not whether the format suffix (.json) was present. A request like /rest/mempool/info would pass this validation but subsequently fail at line 653 with "output format not found" rather than the more specific "Invalid URI format" error here.


However, since this is a backport PR, the implementation should match the upstream Bitcoin Core code. The behavior still returns an appropriate error (just a different message), and there's no crash, undefined behavior, or functional incorrectness. Based on learnings.




623-657: LGTM: Clean consolidation of duplicate endpoints.


The refactoring successfully consolidates the previous separate rest_mempool_info and rest_mempool_contents handlers into a unified rest_mempool function. The implementation correctly:


    

  • Validates the subpath parameter to accept only "contents" or "info"
    • 

  • Dispatches to MempoolToJSON (with verbose=true) for contents
    • 

  • Dispatches to MempoolInfoToJSON for info
    • 

  • Maintains the same behavior as the original separate handlers
    • 



The snake_case naming (str_uri_part, str_json) aligns with modern C++ conventions.




968-968: LGTM: Dispatch table correctly updated.


The routing entry properly maps /rest/mempool/ to the unified handler, preserving the existing endpoint paths:


    

  • /rest/mempool/info.json → str_uri_part = info.json
    • 

  • /rest/mempool/contents.json → str_uri_part = contents.json
    • 






ci/test/00_setup_env_native_qt5.sh (1)


17-17: LGTM!


The boolean flag approach simplifies the control flow and aligns well with the updated download script interface.





ci/dash/test_integrationtests.sh (1)


23-26: LGTM!


The updated control flow correctly implements the boolean flag check and the script invocation properly uses the -b batch mode flag. The changes are consistent with the updated interface in test/get_previous_releases.py.





test/get_previous_releases.py (3)


23-105: LGTM!


The refactored SHA256_SUMS structure from hash -> tarball_string to hash -> {tag, tarball} enables better metadata association and supports the new default tag behavior. The structure is consistent across all entries.




170-171: Validation logic is correct.


The checksum validation correctly handles the updated data structure:


    

  • Line 170 checks both hash existence and tarball name match
    • 

  • Line 171 uses a list comprehension to extract all known tarball names for membership checking
    • 



The logic properly distinguishes between checksum mismatches and unknown versions.




323-328: LGTM!


The argument parser correctly implements the new default behavior:


    

  • Line 323 uses sorted() to ensure deterministic tag ordering
    • 

  • The nargs='*' with default=all_tags enables batch mode to download all releases when no tags are specified
    • 

  • Help text clearly explains the default behavior
    • 



This change supports the simplified -b flag usage documented in the README.





src/node/blockstorage.cpp (1)


449-456: LGTM: minor copy avoidance, no behavior change


The const-ref bind to the blocks directory path is safe within the function and preserves semantics.





src/rpc/mining.cpp (1)


755-762: Remove review comment — the analysis was based on incorrect assumptions


The original review comment incorrectly assumed get_str() returns by value. The actual signature is const std::string& get_str() const, which returns a const reference. The code binding lpstr to this reference is straightforward and safe—there is no temporary lifetime extension or move elision happening here. The reviewer's stated concern is not applicable to this code.





src/bitcoin-cli.cpp (1)


949-956: LGTM: Safe binding; UniValue::get_str() confirmed to return const reference


Verification confirms get_str() returns const std::string&. The const-ref binding to the wallet object's string member is safe; the reference lifetime is tied to the wallet loop variable, and the string is copied when passed to ConnectAndCallRPC. No issues found.





src/outputtype.cpp (1)


21-21: Change verified and safe — no issues found


All downstream assumptions validated:


    

  • Header declaration matches implementation: both std::array<OutputType, 1>
    • 

  • All switch statements properly guard UNKNOWN with explicit cases and asserts
    • 

  • No code iterates OUTPUT_TYPES expecting multiple elements
    • 

  • Tests don't assume OUTPUT_TYPES contains multiple types; BasicOutputTypesTest only exercises LEGACY path
    • 

  • No reliance on array size or range-based loops
    • 






src/key.h (1)


138-138: LGTM! [[nodiscard]] attributes properly enforce return-value checking.


The [[nodiscard]] attributes ensure that callers check the return values of the Derive methods, which can fail due to depth limits or cryptographic operations. This is a good safety improvement that aligns with the explicit failure checks added throughout the codebase.


Also applies to: 189-189





src/wallet/hdchain.cpp (2)


157-165: LGTM! Consistent application of exception-based derivation error handling.


All five derivation steps in the BIP44 path now use the DeriveExtKey wrapper, ensuring that any derivation failure is caught and reported via exception. This is consistent with the [[nodiscard]] enforcement added to the Derive methods.




132-138: Exception handling is correctly implemented—both callers propagate exceptions naturally.


The verification confirms that DeriveChildExtKey calls DeriveExtKey 6 times (lines 157, 159, 161, 163, 165), and any exception thrown by DeriveExtKey propagates to callers. Both call sites are in functions that already use exception-throwing for error handling:


    

  • GetKey (line 1162): Already throws std::runtime_error for validation failures (lines 1169–1171, 1172–1176, 1179–1180). The DeriveChildExtKey exception propagates naturally.
    • 

  • DeriveNewChildKey (line 1281): Throws std::runtime_error for multiple failure conditions (lines 1287–1289, 1291–1295, 1297–1298, 1301–1302, 1323–1324, 1326–1328, 1330–1331). The DeriveChildExtKey exception at line 1308 propagates naturally.
    • 



The new exception-throwing behavior from DeriveExtKey is consistent with the existing exception-aware error handling model in both callers.





src/script/descriptor.cpp (2)


307-307: LGTM! Proper failure propagation for Derive operations.


All Derive call sites now explicitly check return values and propagate failures by returning false. This aligns with the [[nodiscard]] attributes added to the Derive methods and ensures derivation failures are never silently ignored.


Also applies to: 367-367, 373-373, 476-477




550-550: LGTM! Correct removal of unnecessary std::move() calls.


Removing std::move() from these string concatenations is correct. When using operator+= with a local variable, the compiler can optimize without an explicit move, and operator+= takes a const reference anyway. The explicit std::move() provided no benefit here.


Also applies to: 574-574








      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  




  








      

  
          





      

  
        

  

        

  coderabbitai[bot]
  

  
            

              
                    coderabbitai
  bot

              

              reviewed


              
                  
                    Oct 25, 2025
                  
              
              
            


            



                
    View reviewed changes
  


            





          

  
  
  
              

                      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        @coderabbitai

      
            coderabbitai
  bot

      

      

      left a comment




      
    


  



                  

                      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
Actionable comments posted: 2


Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.



⚠️ Outside diff range comments (1)



src/script/miniscript.h (1)


8-14: Missing headers for std::set and std::tie cause build failures with C++20 toolchains.


Include  and  explicitly (pair may also require , but not strictly needed if already available transitively).


 #include <algorithm>
-#include <functional>
+#include <functional>
 #include <numeric>
 #include <memory>
 #include <optional>
 #include <string>
 #include <vector>
+#include <set>
+#include <tuple>
+// (optional) #include <utility>  // for std::pair, if not included elsewhere


Based on learnings.









♻️ Duplicate comments (3)



test/get_previous_releases.py (2)


170-176: Verify that line 171 logic fix is present.


A previous review flagged that line 171's condition if tarball in SHA256_SUMS.values() will always be False (since SHA256_SUMS.values() now returns dict objects, not strings). The comment indicated this was addressed in commit 797e264, but the problematic code still appears to be present in the current version.


Run the following to confirm the current state:


#!/bin/bash
# Check the actual logic on line 171
rg -n -A2 -B2 'if tarball in SHA256_SUMS.values' test/get_previous_releases.py




323-329: Verify that formatting and determinism fixes are present.


A previous review flagged two issues here:


    

  1. Use of set() for default creates nondeterministic ordering
    2. 

  2. Flake8 E126 errors for over-indentation on lines 324-325
    3. 



The comment indicated these were addressed in commit 797e264, but both static analysis (E126 errors) and the code (still using set()) suggest the issues may still be present.


Based on the past review, the fix should use sorted() for deterministic ordering and correct the indentation to satisfy PEP8:


parser.add_argument(
    'tags',
    nargs='*',
    default=sorted([v['tag'] for v in SHA256_SUMS.values()]),
    help=('release tags. e.g.: v19.1.0 v19.0.0-rc.9 '
          '(if not specified, the full list needed for backwards '
          'compatibility tests will be used)'),
)





src/script/miniscript.h (1)


1399-1403: AFTER timelock: compare the contained value, not std::optional (compile fix).


Directly comparing std::optional<int64_t> to integers is ill-formed in C++20. Dereference before comparison, like the OLDER branch above.


-            if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
+            if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
                 in += 2;
-                if (num < 1 || num > 0x7FFFFFFFL) return {};
+                if (*num < 1 || *num > 0x7FFFFFFFL) return {};
                 constructed.push_back(MakeNodeRef<Key>(ctx, Fragment::AFTER, *num));
                 break;
             }


Based on learnings.







🧹 Nitpick comments (1)



src/llmq/snapshot.cpp (1)


204-210: Remove stray semicolon.


Line 204 contains an empty statement (stray semicolon) that serves no purpose. This appears to be leftover from the refactoring and should be removed for code clarity.


Apply this diff to remove the stray semicolon:


         }
 
-        ;
         if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman,
                                        GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus4CIndex,
                                                             use_legacy_construction),







📜 Review details

Configuration used: CodeRabbit UI


Review profile: CHILL


Plan: Pro



📥 Commits

Reviewing files that changed from the base of the PR and between dc5144b and e4ce074.





📒 Files selected for processing (71)

    

  • ci/dash/lint-tidy.sh (1 hunks)
    • 

  • ci/test/00_setup_env_native_qt5.sh (1 hunks)
    • 

  • ci/test/05_before_script.sh (1 hunks)
    • 

  • contrib/guix/libexec/build.sh (1 hunks)
    • 

  • share/setup.nsi.in (2 hunks)
    • 

  • src/.clang-tidy (1 hunks)
    • 

  • src/Makefile.test.include (1 hunks)
    • 

  • src/bench/wallet_loading.cpp (0 hunks)
    • 

  • src/bitcoin-cli.cpp (1 hunks)
    • 

  • src/bitcoin-tx.cpp (1 hunks)
    • 

  • src/blockfilter.cpp (1 hunks)
    • 

  • src/coins.cpp (1 hunks)
    • 

  • src/core_read.cpp (1 hunks)
    • 

  • src/evo/specialtxman.cpp (1 hunks)
    • 

  • src/init.cpp (1 hunks)
    • 

  • src/instantsend/instantsend.cpp (1 hunks)
    • 

  • src/key.cpp (1 hunks)
    • 

  • src/key.h (2 hunks)
    • 

  • src/llmq/blockprocessor.cpp (1 hunks)
    • 

  • src/llmq/snapshot.cpp (4 hunks)
    • 

  • src/logging.cpp (1 hunks)
    • 

  • src/masternode/meta.cpp (1 hunks)
    • 

  • src/netbase.cpp (3 hunks)
    • 

  • src/node/blockstorage.cpp (1 hunks)
    • 

  • src/outputtype.cpp (1 hunks)
    • 

  • src/outputtype.h (1 hunks)
    • 

  • src/pubkey.cpp (1 hunks)
    • 

  • src/pubkey.h (2 hunks)
    • 

  • src/qt/splashscreen.cpp (1 hunks)
    • 

  • src/rest.cpp (2 hunks)
    • 

  • src/rpc/blockchain.cpp (1 hunks)
    • 

  • src/rpc/mining.cpp (1 hunks)
    • 

  • src/rpc/output_script.cpp (1 hunks)
    • 

  • src/rpc/quorums.cpp (1 hunks)
    • 

  • src/rpc/rawtransaction_util.cpp (1 hunks)
    • 

  • src/rpc/util.cpp (1 hunks)
    • 

  • src/script/descriptor.cpp (6 hunks)
    • 

  • src/script/miniscript.cpp (5 hunks)
    • 

  • src/script/miniscript.h (33 hunks)
    • 

  • src/script/script.h (1 hunks)
    • 

  • src/test/base58_tests.cpp (2 hunks)
    • 

  • src/test/bip32_tests.cpp (1 hunks)
    • 

  • src/test/blockfilter_tests.cpp (1 hunks)
    • 

  • src/test/descriptor_tests.cpp (2 hunks)
    • 

  • src/test/fuzz/miniscript.cpp (1 hunks)
    • 

  • src/test/fuzz/miniscript_decode.cpp (0 hunks)
    • 

  • src/test/fuzz/txorphan.cpp (0 hunks)
    • 

  • src/test/key_io_tests.cpp (3 hunks)
    • 

  • src/test/miniscript_tests.cpp (3 hunks)
    • 

  • src/test/script_tests.cpp (1 hunks)
    • 

  • src/test/sighash_tests.cpp (1 hunks)
    • 

  • src/test/transaction_tests.cpp (4 hunks)
    • 

  • src/test/util_tests.cpp (1 hunks)
    • 

  • src/test/validation_block_tests.cpp (1 hunks)
    • 

  • src/univalue/include/univalue.h (1 hunks)
    • 

  • src/univalue/lib/univalue.cpp (2 hunks)
    • 

  • src/util/message.cpp (0 hunks)
    • 

  • src/util/strencodings.cpp (0 hunks)
    • 

  • src/util/string.cpp (1 hunks)
    • 

  • src/util/string.h (0 hunks)
    • 

  • src/util/threadinterrupt.h (1 hunks)
    • 

  • src/wallet/bdb.cpp (1 hunks)
    • 

  • src/wallet/hdchain.cpp (2 hunks)
    • 

  • src/wallet/receive.cpp (1 hunks)
    • 

  • src/wallet/scriptpubkeyman.cpp (1 hunks)
    • 

  • src/wallet/spend.cpp (1 hunks)
    • 

  • src/wallet/walletdb.cpp (2 hunks)
    • 

  • test/README.md (1 hunks)
    • 

  • test/functional/rpc_signmessagewithprivkey.py (2 hunks)
    • 

  • test/get_previous_releases.py (3 hunks)
    • 

  • test/lint/run-lint-format-strings.py (0 hunks)
    • 






💤 Files with no reviewable changes (7)

    

  • src/bench/wallet_loading.cpp
    • 

  • src/util/string.h
    • 

  • src/test/fuzz/miniscript_decode.cpp
    • 

  • test/lint/run-lint-format-strings.py
    • 

  • src/test/fuzz/txorphan.cpp
    • 

  • src/util/strencodings.cpp
    • 

  • src/util/message.cpp
    • 






🚧 Files skipped from review as they are similar to previous changes (28)

    

  • src/Makefile.test.include
    • 

  • src/test/key_io_tests.cpp
    • 

  • src/.clang-tidy
    • 

  • src/logging.cpp
    • 

  • src/util/string.cpp
    • 

  • src/wallet/walletdb.cpp
    • 

  • contrib/guix/libexec/build.sh
    • 

  • src/core_read.cpp
    • 

  • share/setup.nsi.in
    • 

  • ci/dash/lint-tidy.sh
    • 

  • src/test/blockfilter_tests.cpp
    • 

  • ci/test/05_before_script.sh
    • 

  • src/wallet/spend.cpp
    • 

  • src/wallet/scriptpubkeyman.cpp
    • 

  • src/blockfilter.cpp
    • 

  • src/key.h
    • 

  • src/rpc/mining.cpp
    • 

  • src/qt/splashscreen.cpp
    • 

  • src/init.cpp
    • 

  • src/script/script.h
    • 

  • src/test/descriptor_tests.cpp
    • 

  • src/wallet/bdb.cpp
    • 

  • src/bitcoin-tx.cpp
    • 

  • src/test/fuzz/miniscript.cpp
    • 

  • src/test/bip32_tests.cpp
    • 

  • src/bitcoin-cli.cpp
    • 

  • src/outputtype.h
    • 

  • src/coins.cpp
    • 






🧰 Additional context used


📓 Path-based instructions (6)


src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1

Dash uses unordered_lru_cache for efficient caching with LRU eviction




Files:


    

  • src/test/util_tests.cpp
    • 

  • src/evo/specialtxman.cpp
    • 

  • src/rpc/util.cpp
    • 

  • src/rpc/blockchain.cpp
    • 

  • src/key.cpp
    • 

  • src/wallet/hdchain.cpp
    • 

  • src/univalue/include/univalue.h
    • 

  • src/test/transaction_tests.cpp
    • 

  • src/instantsend/instantsend.cpp
    • 

  • src/test/sighash_tests.cpp
    • 

  • src/node/blockstorage.cpp
    • 

  • src/netbase.cpp
    • 

  • src/outputtype.cpp
    • 

  • src/script/miniscript.cpp
    • 

  • src/rpc/rawtransaction_util.cpp
    • 

  • src/test/base58_tests.cpp
    • 

  • src/rpc/quorums.cpp
    • 

  • src/script/descriptor.cpp
    • 

  • src/test/validation_block_tests.cpp
    • 

  • src/masternode/meta.cpp
    • 

  • src/pubkey.h
    • 

  • src/univalue/lib/univalue.cpp
    • 

  • src/test/script_tests.cpp
    • 

  • src/wallet/receive.cpp
    • 

  • src/util/threadinterrupt.h
    • 

  • src/script/miniscript.h
    • 

  • src/pubkey.cpp
    • 

  • src/rest.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 

  • src/rpc/output_script.cpp
    • 

  • src/llmq/blockprocessor.cpp
    • 

  • src/llmq/snapshot.cpp
    • 






src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests




Files:


    

  • src/test/util_tests.cpp
    • 

  • src/test/transaction_tests.cpp
    • 

  • src/test/sighash_tests.cpp
    • 

  • src/test/base58_tests.cpp
    • 

  • src/test/validation_block_tests.cpp
    • 

  • src/test/script_tests.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 






src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




Masternode lists must use immutable data structures (Immer library) for thread safety




Files:


    

  • src/evo/specialtxman.cpp
    • 

  • src/masternode/meta.cpp
    • 






src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/**

📄 CodeRabbit inference engine (CLAUDE.md)




Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue




Files:


    

  • src/univalue/include/univalue.h
    • 

  • src/univalue/lib/univalue.cpp
    • 






test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)




Functional tests should be written in Python and placed in test/functional/




Files:


    

  • test/functional/rpc_signmessagewithprivkey.py
    • 






ci/**

📄 CodeRabbit inference engine (CLAUDE.md)




Unless specifically prompted, avoid making changes to the ci directory (continuous integration)




Files:


    

  • ci/test/00_setup_env_native_qt5.sh
    • 






🧠 Learnings (5)


📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.



Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.






📚 Learning: 2025-07-20T18:42:49.794Z

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue



Applied to files:


    

  • src/univalue/include/univalue.h
    • 






📚 Learning: 2025-10-25T07:08:51.889Z

Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.



Applied to files:


    

  • src/univalue/include/univalue.h
    • 






📚 Learning: 2025-10-25T07:08:51.889Z

Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.



Applied to files:


    

  • src/univalue/include/univalue.h
    • 






📚 Learning: 2025-10-05T20:38:28.457Z

Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.



Applied to files:


    

  • src/univalue/include/univalue.h
    • 






🧬 Code graph analysis (11)


src/rpc/blockchain.cpp (3)



src/test/script_tests.cpp (1)

    

  • script (286-286)
    • 






src/wallet/interfaces.cpp (4)

    

  • script (188-195)
    • 

  • script (188-188)
    • 

  • script (204-208)
    • 

  • script (204-204)
    • 






src/script/descriptor.cpp (2)

    

  • scripts (758-763)
    • 

  • scripts (758-758)
    • 








src/univalue/include/univalue.h (1)



src/univalue/lib/univalue.cpp (12)

    

  • push_back (104-109)
    • 

  • push_back (104-104)
    • 

  • push_backV (111-116)
    • 

  • push_backV (111-111)
    • 

  • __pushKV (118-124)
    • 

  • __pushKV (118-118)
    • 

  • key (187-197)
    • 

  • key (187-187)
    • 

  • pushKV (126-135)
    • 

  • pushKV (126-126)
    • 

  • pushKVs (137-143)
    • 

  • pushKVs (137-137)
    • 








src/script/miniscript.cpp (3)



src/script/script.cpp (2)

    

  • CheckMinimalPush (386-409)
    • 

  • CheckMinimalPush (386-386)
    • 






src/script/standard.h (1)

    

  • IsPushdataOp (112-115)
    • 






src/script/script.h (2)

    

  • CScriptNum (211-226)
    • 

  • CScriptNum (230-240)
    • 








src/rpc/rawtransaction_util.cpp (1)



src/rpc/request.cpp (2)

    

  • JSONRPCError (56-62)
    • 

  • JSONRPCError (56-56)
    • 








src/rpc/quorums.cpp (2)



src/llmq/snapshot.cpp (2)

    

  • BuildQuorumRotationInfo (20-312)
    • 

  • BuildQuorumRotationInfo (20-23)
    • 






src/rpc/request.cpp (2)

    

  • JSONRPCError (56-62)
    • 

  • JSONRPCError (56-56)
    • 








src/pubkey.h (1)



src/pubkey.cpp (4)

    

  • Derive (240-259)
    • 

  • Derive (240-240)
    • 

  • Derive (311-318)
    • 

  • Derive (311-311)
    • 








test/functional/rpc_signmessagewithprivkey.py (1)



test/functional/test_framework/util.py (2)

    

  • assert_equal (69-74)
    • 

  • assert_raises_rpc_error (132-148)
    • 








src/script/miniscript.h (3)



src/script/miniscript.cpp (10)

    

  • ComputeType (36-248)
    • 

  • ComputeType (36-36)
    • 

  • ComputeScriptLen (250-280)
    • 

  • ComputeScriptLen (250-250)
    • 

  • FindNextChar (331-339)
    • 

  • FindNextChar (331-331)
    • 

  • DecomposeScript (282-316)
    • 

  • DecomposeScript (282-282)
    • 

  • ParseScriptNumber (318-329)
    • 

  • ParseScriptNumber (318-318)
    • 






src/test/miniscript_tests.cpp (6)

    

  • a (74-76)
    • 

  • a (74-74)
    • 

  • key (79-79)
    • 

  • key (79-79)
    • 

  • key (82-87)
    • 

  • key (82-82)
    • 






src/test/fuzz/miniscript.cpp (10)

    

  • a (50-52)
    • 

  • a (50-50)
    • 

  • a (97-99)
    • 

  • a (97-97)
    • 

  • key (54-60)
    • 

  • key (54-54)
    • 

  • key (101-105)
    • 

  • key (101-101)
    • 

  • key (107-112)
    • 

  • key (107-107)
    • 








src/rest.cpp (1)



src/rpc/mempool.cpp (4)

    

  • MempoolToJSON (332-371)
    • 

  • MempoolToJSON (332-332)
    • 

  • MempoolInfoToJSON (682-699)
    • 

  • MempoolInfoToJSON (682-682)
    • 








src/test/miniscript_tests.cpp (1)



src/test/fuzz/miniscript.cpp (12)

    

  • a (50-52)
    • 

  • a (50-50)
    • 

  • a (97-99)
    • 

  • a (97-97)
    • 

  • key (54-60)
    • 

  • key (54-54)
    • 

  • key (101-105)
    • 

  • key (101-101)
    • 

  • key (107-112)
    • 

  • key (107-107)
    • 

  • FromString (63-68)
    • 

  • FromString (63-63)
    • 








src/llmq/snapshot.cpp (1)



src/evo/smldiff.cpp (2)

    

  • BuildSimplifiedMNListDiff (153-224)
    • 

  • BuildSimplifiedMNListDiff (153-156)
    • 








🪛 Flake8 (7.3.0)


test/get_previous_releases.py

[error] 324-324: continuation line over-indented for hanging indent


(E126)




[error] 325-325: continuation line over-indented for hanging indent


(E126)







🪛 GitHub Actions: Clang Diff Format Check


src/wallet/hdchain.cpp

[error] 130-132: Clang format differences found. Run 'clang-format-diff.py -p1' to apply formatting changes in this file.







🪛 Gitleaks (8.28.0)


test/functional/rpc_signmessagewithprivkey.py

[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.


(generic-api-key)







🪛 markdownlint-cli2 (0.18.1)


test/README.md

105-105: Fenced code blocks should have a language specified


(MD040, fenced-code-language)









⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)

    

  • GitHub Check: win64-build / Build source
    • 

  • GitHub Check: mac-build / Build source
    • 

  • GitHub Check: arm-linux-build / Build source
    • 

  • GitHub Check: linux64_fuzz-build / Build source
    • 

  • GitHub Check: linux64-build / Build source
    • 

  • GitHub Check: linux64_ubsan-build / Build source
    • 

  • GitHub Check: linux64_sqlite-build / Build source
    • 

  • GitHub Check: Lint / Run linters
    • 






🔇 Additional comments (52)



src/rpc/output_script.cpp (1)


38-41: LGTM! Stylistic consistency with upstream Bitcoin Core.


The comment format change from /* optional */ to /*optional=*/ aligns with Bitcoin Core's named-parameter comment convention. No functional changes are introduced.





src/rpc/util.cpp (1)


102-102: LGTM! Efficient const-reference binding.


The change from by-value to const-reference binding avoids an unnecessary string copy while maintaining correctness. The reference remains valid throughout its usage since v stays in scope and unmodified.





src/test/util_tests.cpp (1)


1307-1307: LGTM! Appropriate lint suppressions for intentional use-after-move patterns.


The NOLINT comments correctly suppress use-after-move warnings for test assertions that intentionally verify origin pointer tracking after move operations. The test only compares addresses of moved-from objects without dereferencing them, which is safe and necessary for validating the Tracker type's origin tracking behavior.


Based on learnings


Also applies to: 1313-1313, 1321-1321





src/netbase.cpp (1)


393-393: LGTM! Clean refactoring to remove unnecessary variable assignments.


The changes eliminate intermediate storage to recvr when the specific error type is not needed, performing direct comparisons instead. The logic is preserved and the code is slightly more concise. The recvr variable is correctly retained where the specific error value must be examined (lines 441-442, 465-479).


Also applies to: 416-416, 482-482





src/llmq/blockprocessor.cpp (1)


441-462: LGTM! Proper move semantics enabled.


Making opt_qc non-const (line 441) correctly enables move semantics for qc.commitment at line 462. If opt_qc were const, the dereferencing at line 447 would yield a const reference, causing std::move to perform a copy instead of an actual move. This change optimizes performance by avoiding unnecessary copying of the commitment data.





src/instantsend/instantsend.cpp (1)


193-193: LGTM! Performance optimization to enable move semantics.


Changing from const auto& to auto& enables std::move(nodeid_islptr_pair) on line 199, avoiding unnecessary copies of the pair containing the InstantSendLockPtr. This is safe because the moved-from elements are immediately erased (lines 203-205), and the pattern correctly avoids iterator invalidation by collecting keys first.





src/rpc/quorums.cpp (1)


953-960: LGTM! Good lock scope optimization.


This change correctly narrows the critical section to only cover the BuildQuorumRotationInfo call that requires cs_main, releasing the lock before the JSON serialization. This reduces lock contention and improves concurrency without affecting correctness.





src/evo/specialtxman.cpp (1)


489-489: LGTM—optimization using swap.


Replacing move assignment with std::swap is semantically equivalent here since mnListRet is always default-constructed at the call site (line 592). The swap approach may offer a marginal performance benefit by avoiding unnecessary destruction of the empty out-parameter.





src/util/threadinterrupt.h (1)


9-9: LGTM! Explicit dependency for thread-safety annotations.


Adding this include makes the dependency on thread-safety annotations explicit, which is good practice. The class uses EXCLUSIVE_LOCKS_REQUIRED on lines 26 and 28, and this include ensures those annotations are properly defined without relying on transitive includes through sync.h.





src/outputtype.cpp (1)


21-21: OUTPUT_TYPES array change from 2 to 1 element is safe and correctly applied.


Verification confirms that no code depends on the previous array size or content. The header and implementation files are consistent (both declare size 1), and there are no hardcoded size assumptions, iterations, or direct access patterns affected by this change.





test/functional/rpc_signmessagewithprivkey.py (1)


37-44: LGTM! Enhanced test coverage with positive and negative cases.


The test properly validates that:


    

  • Message verification succeeds with the P2PKH address derived from the private key
    • 

  • Message verification fails with appropriate error for non-P2PKH addresses (SH(PKH))
    • 



The hardcoded address check on line 39 is good practice, ensuring deterministic address derivation.





src/llmq/snapshot.cpp (3)


154-158: LGTM!


The refactoring correctly assigns the snapshot to response.quorumSnapshotAtHMinusC and checks the same field for validity.




184-188: LGTM!


The refactoring correctly assigns the snapshot to response.quorumSnapshotAtHMinus3C and checks the same field for validity.




198-202: LGTM!


The refactoring correctly assigns the snapshot to response.quorumSnapshotAtHMinus4C and checks the same field for validity.





src/masternode/meta.cpp (1)


150-154: No issues found—change is correct and valid.


The m_seen_platform_bans member uses unordered_lru_cache<uint256, PlatformBanMessage, StaticSaltedHasher>, and the emplace method with signature void emplace(const Key& key, Value&& v) exists. The change from insert to emplace correctly leverages the rvalue reference parameter (PlatformBanMessage&&) for optimal move semantics. The code will compile without issues and follows Dash coding guidelines for cache usage.





src/rpc/blockchain.cpp (1)


2495-2499: LGTM! Valid optimization to reduce copying.


The change from const auto& to CScript& enables the move operation on line 2498, which is a reasonable optimization. The script is copied into needles (line 2497) where it's needed for scanning, then moved into descriptors (line 2498) to avoid a second copy. While this leaves the scripts vector with moved-from elements, this is acceptable since the vector is not used after the loop completes.





src/node/blockstorage.cpp (1)


451-451: LGTM – valid optimization to avoid unnecessary copying.


The const reference binding avoids a copy when capturing the temporary returned by GetBlocksDirPath(). The function declaration in src/util/system.h:302 shows it returns fs::path by value, and binding a const reference to this temporary is safe because const references extend the temporary's lifetime for the scope of the for-loop. This optimization is idiomatic C++ and eliminates an unnecessary copy operation.





src/rest.cpp (2)


623-657: LGTM - URI validation logic is correct with two-stage validation.


Regarding the past review comment about URI validation: the implementation uses a two-stage validation approach that is correct:


    

  1. Line 630-632: Validates the sub-path component (param) is either "contents" or "info"
    2. 

  2. Line 637-656: Validates the format suffix is JSON (or returns "output format not found" for UNDEF/other formats)
    3. 



This means:


    

  • /rest/mempool/info.json → passes both validations ✓
    • 

  • /rest/mempool/info (no suffix) → passes line 630, fails at line 653 with "output format not found"
    • 

  • /rest/mempool/foo.json → fails at line 631 with "Invalid URI format"
    • 



The error message at line 631 correctly indicates .json is expected, even though this specific check only validates the sub-path. The format validation happens subsequently in the switch statement, which is a sound design.


Based on learnings (this is a backport PR and the logic matches expected upstream behavior).




968-968: LGTM - Dispatch table correctly updated for unified handler.


The dispatch table entry correctly routes /rest/mempool/ to the new unified rest_mempool handler, consolidating the previous separate entries for info and contents endpoints.





test/README.md (2)


103-107: Documentation updated correctly.


The command usage has been simplified to use the -b flag without explicit version arguments, which aligns with the refactored implementation that uses a boolean flag approach.




109-110: Clear explanatory text added.


The additional sentence clarifies the purpose of the command for users.





ci/test/00_setup_env_native_qt5.sh (1)


17-17: Environment variable refactored to boolean flag.


The change from a version list to a boolean flag simplifies the CI configuration and aligns with the broader refactoring in this PR.





test/get_previous_releases.py (1)


23-105: SHA256_SUMS structure enhanced.


The refactoring from a simple hash-to-string mapping to a hash-to-dict structure with separate tag and tarball fields improves data organization and enables better association of version metadata throughout the script.





src/test/script_tests.cpp (1)


884-884: LGTM: Efficient const-reference binding


Changing from by-value to const-reference avoids unnecessary copying of the UniValue object while maintaining the same semantics.





src/test/validation_block_tests.cpp (1)


185-185: LGTM: Avoid unnecessary shared_ptr copies


Using const-reference iteration prevents copying shared_ptr objects on each iteration, improving performance without changing behavior.





src/key.cpp (1)


344-344: LGTM: Necessary depth overflow guard


This guard prevents overflow when nDepth is at the maximum value (255). Without it, incrementing on line 345 would wrap to 0, breaking the depth tracking invariant. The early return properly signals derivation failure at maximum depth.





src/test/base58_tests.cpp (1)


27-27: LGTM: Efficient const-reference binding in test loops


Both changes avoid unnecessary UniValue copies while maintaining identical test behavior. The const-reference binding is safe since the test variable is only used for read operations.


Also applies to: 49-49





src/test/transaction_tests.cpp (1)


156-156: LGTM: Consistent const-reference optimization across test cases


All four changes avoid unnecessary UniValue copies in the test loops. The const-reference binding is safe since these variables are only used for read-only operations within their respective scopes.


Also applies to: 175-175, 240-240, 259-259





src/wallet/receive.cpp (1)


431-431: LGTM: Avoid expensive set copies


Using const-reference iteration prevents copying potentially large std::set<CTxDestination> objects on each iteration. The reference is only used for read operations, making this a safe optimization.





src/script/descriptor.cpp (4)


307-307: LGTM: Proper error handling replaces assertions


Replacing assertions with explicit error checks and early returns is a significant improvement. This allows derivation failures (including depth overflow from the guards added in key.cpp) to be properly propagated instead of crashing the application.


Also applies to: 367-367




373-373: LGTM: Check derivation result


This ensures that derivation failures are caught and propagated, aligning with the [[nodiscard]] attribute on Derive methods.




476-477: LGTM: Consistent error handling for private key derivation


Both branches now properly check derivation results and return false on failure, ensuring consistent error handling across UNHARDENED and HARDENED derivation paths.




550-550: LGTM: Appropriate handling of temporaries


Removing std::move on variables that are about to be destroyed anyway (like tmp at end of scope) is appropriate. The compiler can perform copy elision or automatic move when beneficial. Similarly, avoiding the move on out in the Merge call is correct since out is reused.


Also applies to: 574-574, 622-622





src/pubkey.cpp (1)


312-312: LGTM: Depth overflow guard for public key derivation


This guard mirrors the one added in CExtKey::Derive (src/key.cpp), preventing overflow when deriving public keys at maximum depth. The consistency between private and public key derivation is important for maintaining invariants.





src/test/sighash_tests.cpp (1)


164-164: LGTM: Efficient const reference binding.


The change from by-value to const reference avoids unnecessary copying of the UniValue object during iteration while maintaining const-correctness.





src/rpc/rawtransaction_util.cpp (1)


149-149: LGTM: Efficient const reference bindings.


Both changes avoid unnecessary copies by binding directly to the const references returned by get_array() and get_obj().


Also applies to: 156-156





src/wallet/hdchain.cpp (2)


132-137: LGTM: Clean exception-based error handling.


The static helper properly converts derivation failures into exceptions, ensuring errors are not silently ignored. This aligns well with the [[nodiscard]] attributes added to the Derive methods.




156-164: LGTM: Consistent error handling across derivation steps.


All derivation steps now use the DeriveExtKey wrapper for uniform exception-based error handling. The BIP44 derivation path remains unchanged.





src/pubkey.h (1)


211-211: LGTM: Proper use of [[nodiscard]] attribute.


Adding [[nodiscard]] to both Derive methods ensures callers explicitly handle potential derivation failures, preventing silent errors.


Also applies to: 284-284





src/univalue/include/univalue.h (1)


81-88: LGTM: Move semantics optimization.


The signature changes to pass-by-value enable move semantics, allowing callers to efficiently transfer ownership. This is a standard modern C++ optimization pattern and matches the upstream Bitcoin Core implementation.


Based on learnings.





src/univalue/lib/univalue.cpp (4)


104-109: LGTM: Clean move semantics implementation.


The by-value parameter with move into the internal vector enables efficient transfers when callers use std::move.




118-124: LGTM: Efficient key-value insertion.


Both parameters are moved into internal storage, enabling efficient insertion for both new keys and values.




126-135: Code is correct: No use-after-move issue.


Regarding the past review comment concern about Line 131: The findKey call occurs before any move operations, and the if-else branches are mutually exclusive. In the true branch (Line 132), key is never moved. In the false branch (Line 134), key is moved but never used afterward. The order of operations is safe.




137-143: LGTM: Efficient batch insertion.


The implementation efficiently moves key-value pairs from the source object into the current object.





src/test/miniscript_tests.cpp (3)


74-76: LGTM: Comparison operator for key ordering.


The KeyCompare method enables key ordering operations needed for duplicate detection, consistent with the pattern used in fuzz tests.




91-113: LGTM: Idiomatic optional-based error handling.


The conversion from boolean returns to std::optional<Key> provides clearer success/failure semantics and aligns with modern C++ practices and the broader PR pattern.




282-294: LGTM: Comprehensive duplicate key detection tests.


The new tests thoroughly validate that miniscripts with duplicate keys are correctly identified as insane across various structures (conjunctions, disjunctions, thresholds) and tree positions.





src/script/miniscript.cpp (5)


20-33: LGTM: Formatting consistency.


The assert statement updates maintain the same type validation logic with improved formatting consistency.




36-248: LGTM: Clean Fragment-based refactor.


The refactor from nodetype to Fragment parameter maintains all type computation logic while improving code clarity. The exhaustive switch statement covers all fragment types.




250-280: LGTM: Consistent Fragment-based refactor.


The script length computation properly uses the Fragment enum with all cases covered and logic preserved.




282-316: LGTM: Modern optional-based error handling.


The refactor to return std::optional<std::vector<Opcode>> provides clearer success/failure semantics compared to the previous boolean return pattern.




318-329: LGTM: Clean optional-based parsing.


The ParseScriptNumber refactor provides clear success/failure semantics through std::optional<int64_t>, properly handling the OP_0 special case, minimal push validation, and script number parsing errors.








      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  
            
  
    
    

              

          
                src/llmq/snapshot.cpp

              
                Outdated
          
        


      
            

      

      Comment on lines

    
      169
     to 
      173
    

  


  


  

    
    

          


              

              

            
                  response.quorumSnapshotAtHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);

            
          

          


              

              

            
                  if (!response.quorumSnapshotAtHMinus2.has_value()) {

            
          

          


              

              

            
                      errorRet = strprintf("Can not find quorum snapshot at H-2C");

            
          

          


              

              

            
                      return false;

            
          

          


              

              

            
                  } else {

            
          

          


              

              

            
                      response.quorumSnapshotAtHMinus2C = std::move(snapshotHMinus2C.value());

            
          

          


              

              

            
                  }

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
⚠️ Potential issue | 🔴 Critical


Fix field name typo in has_value() check.


Line 170 checks response.quorumSnapshotAtHMinus2.has_value() but should check response.quorumSnapshotAtHMinus2C.has_value() to match the field assigned on line 169. This typo will cause a compilation error or logic bug.


Apply this diff to fix the field name:


     response.quorumSnapshotAtHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);
-    if (!response.quorumSnapshotAtHMinus2.has_value()) {
+    if (!response.quorumSnapshotAtHMinus2C.has_value()) {
         errorRet = strprintf("Can not find quorum snapshot at H-2C");
         return false;
     }




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



  

    

      

        Suggested change
      

    

    

      

          

            
                response.quorumSnapshotAtHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);
          

          

            
                if (!response.quorumSnapshotAtHMinus2.has_value()) {
          

          

            
                    errorRet = strprintf("Can not find quorum snapshot at H-2C");
          

          

            
                    return false;
          

          

            
                } else {
          

          

            
                    response.quorumSnapshotAtHMinus2C = std::move(snapshotHMinus2C.value());
          

          

            
                }
          

          

            
                response.quorumSnapshotAtHMinus2C = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinus2CIndex);
          

          

            
                if (!response.quorumSnapshotAtHMinus2C.has_value()) {
          

          

            
                    errorRet = strprintf("Can not find quorum snapshot at H-2C");
          

          

            
                    return false;
          

          

            
                }
          

      

    

    

  







🤖 Prompt for AI Agents

In src/llmq/snapshot.cpp around lines 169 to 173, there's a typo in the
has_value() check: the code assigns to response.quorumSnapshotAtHMinus2C but
checks response.quorumSnapshotAtHMinus2; change the check to
response.quorumSnapshotAtHMinus2C.has_value() so the existence test matches the
assigned field and prevents the logic/compile error.





    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                test/functional/rpc_signmessagewithprivkey.py

          
        


      
            

      

      Comment on lines

    
      +22
     to 
      +26
    

  


  


  

    
    

          


              

              

            
                  def addresses_from_privkey(self, priv_key):

            
          

          


              

              

            
                      '''Return addresses for a given WIF private key in legacy (P2PKH),

            
          

          


              

              

            
                         and script-to-hash (SH) forman.'''

            
          

          


              

              

            
                      descriptors = f'pkh({priv_key})' , f'sh(pkh({priv_key}))'

            
          

          


              

              

            
                      return [self.nodes[0].deriveaddresses(descsum_create(desc))[0] for desc in descriptors]

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
⚠️ Potential issue | 🟡 Minor


Fix typo in docstring.


Line 24 contains a typo: "forman" should be "format".


Apply this diff:


-    def addresses_from_privkey(self, priv_key):
-        '''Return addresses for a given WIF private key in legacy (P2PKH),
-           and script-to-hash (SH) forman.'''
+    def addresses_from_privkey(self, priv_key):
+        '''Return addresses for a given WIF private key in legacy (P2PKH),
+           and script-to-hash (SH) format.'''




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



  

    

      

        Suggested change
      

    

    

      

          

            
                def addresses_from_privkey(self, priv_key):
          

          

            
                    '''Return addresses for a given WIF private key in legacy (P2PKH),
          

          

            
                       and script-to-hash (SH) forman.'''
          

          

            
                    descriptors = f'pkh({priv_key})' , f'sh(pkh({priv_key}))'
          

          

            
                    return [self.nodes[0].deriveaddresses(descsum_create(desc))[0] for desc in descriptors]
          

          

            
                def addresses_from_privkey(self, priv_key):
          

          

            
                    '''Return addresses for a given WIF private key in legacy (P2PKH),
          

          

            
                       and script-to-hash (SH) format.'''
          

          

            
                    descriptors = f'pkh({priv_key})' , f'sh(pkh({priv_key}))'
          

          

            
                    return [self.nodes[0].deriveaddresses(descsum_create(desc))[0] for desc in descriptors]
          

      

    

    

  







🤖 Prompt for AI Agents

In test/functional/rpc_signmessagewithprivkey.py around lines 22 to 26, the
docstring for addresses_from_privkey has a typo: "forman" should be "format";
update the docstring to read "...and script-to-hash (SH) format." keeping the
rest of the string unchanged.





    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  








  








      

  
          

  
  

  
      @knst
  knst


        marked this pull request as draft

    October 25, 2025 20:23    
    




  
    

    

      
    

    


      


          @knst
knst




        changed the title
backport: bitcoin#23703, #24419, #24860, #25466, #25642, #25650, #25707, #25709, #25733, #25760, #25829, #25869, #25872, #26589

backport: bitcoin#23703, #24860, #25642, #25650, #25760, #25829, #25869, #26589


      Oct 26, 2025

    

  



  
  

  
  

  
      @knst
  knst


        marked this pull request as ready for review

    October 26, 2025 13:59    
    




  





      

  
        

  

        

  greptile-apps[bot]
  

  
            

              
                    greptile-apps
  bot

              

              reviewed


              
                  
                    Oct 26, 2025
                  
              
              
            


            



                
    View reviewed changes
  


            





          

  
  
  
              

                      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        @greptile-apps

      
            greptile-apps
  bot

      

      

      left a comment




      
    


  



                  

                      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
Greptile Overview


Greptile Summary


This PR backports eight Bitcoin Core v24 commits to Dash, focusing on key derivation safety, miniscript improvements, build system enhancements, and testing infrastructure updates. The changes add explicit error handling for BIP32 hierarchical deterministic key derivation operations throughout the wallet and descriptor code, replacing unsafe assertions and unchecked calls with proper return value validation. The miniscript implementation receives significant updates including duplicate key detection, safer key converter APIs using std::optional, and new fuzz testing infrastructure. Build and packaging improvements include adding share/rpcauth to distribution tarballs and updating the NSIS Windows installer. The testing infrastructure is modernized with automatic previous release detection in test/get_previous_releases.py, eliminating the need to manually specify version lists in CI configuration files. Code style is standardized with [[nodiscard]] attributes on critical derivation methods and modern comment syntax in RPC definitions. The changes integrate cleanly with Dash's architecture while maintaining compatibility with existing masternode, LLMQ, and governance systems that depend on BIP32 key derivation.


Important Files Changed






Filename
Score
Overview






src/key.cpp
5/5
Adds depth overflow guard preventing BIP32 derivation at maximum depth 255




src/key.h
5/5
Adds [[nodiscard]] attributes to Derive methods enforcing error checking




src/pubkey.h
5/5
Adds [[nodiscard]] attributes to public key derivation methods




src/pubkey.cpp
5/5
Adds matching depth overflow check for public key derivation




src/wallet/hdchain.cpp
4/5
Introduces DeriveExtKey wrapper that throws on derivation failure instead of silently continuing




src/script/descriptor.cpp
4/5
Replaces assertions with proper error propagation for all BIP32 derivation calls




src/test/bip32_tests.cpp
5/5
Adds comprehensive test coverage for maximum depth boundary conditions




src/test/descriptor_tests.cpp
5/5
Updates tests to explicitly verify derivation success with BOOST_CHECK assertions




src/script/miniscript.cpp
3/5
Large refactoring of miniscript parsing with new duplicate key detection logic




src/script/miniscript.h
4/5
Updates key converter interface to use std::optional and adds KeyCompare method




src/test/miniscript_tests.cpp
5/5
Adds extensive tests for duplicate key detection in miniscript structures




test/fuzz/miniscript.cpp
3/5
New fuzz test with assertions that could crash on invalid inputs




test/fuzz/miniscript_decode.cpp
5/5
File removed as part of miniscript refactoring




src/Makefile.test.include
5/5
Updates fuzz test source reference from miniscript_decode.cpp to miniscript.cpp




src/script/script.h
5/5
Fixes counter increment ordering bug in BuildScript template




test/get_previous_releases.py
4/5
Restructures SHA256_SUMS to enable automatic tag inference for CI testing




ci/test/00_setup_env_native_qt5.sh
4/5
Replaces hardcoded version list with boolean flag for cleaner CI configuration




ci/test/05_before_script.sh
4/5
Updates conditional logic to use boolean DOWNLOAD_PREVIOUS_RELEASES flag




test/README.md
5/5
Simplifies documentation for running backwards compatibility tests




test/functional/rpc_signmessagewithprivkey.py
5/5
Adds explicit test coverage for P2PKH vs non-P2PKH address validation




contrib/guix/libexec/build.sh
5/5
Includes share/rpcauth directory in binary distribution tarballs




share/setup.nsi.in
4/5
Updates Windows installer to include dash.conf example and rpcauth scripts




src/rpc/output_script.cpp
5/5
Updates comment syntax to modern named-parameter format




src/outputtype.h
5/5
Reduces OUTPUT_TYPES array size from 2 to 1 reflecting removed SegWit types




src/outputtype.cpp
5/5
Removes UNKNOWN from OUTPUT_TYPES enumeration array




src/rest.cpp
4/5
Consolidates two mempool REST endpoints into single unified handler






Confidence score: 3/5


    

  • This PR requires careful review due to substantial changes in critical cryptographic code paths and potential undefined behavior in new fuzz tests.
    • 

  • Score reflects three main concerns: (1) miniscript.cpp changes are large and complex with potential edge cases in the new duplicate key detection algorithm; (2) new fuzz test in test/fuzz/miniscript.cpp contains assertions that will crash on invalid inputs rather than handling them gracefully; (3) rest.cpp endpoint consolidation has minor URI validation ordering issues that could produce confusing error messages.
    • 

  • Pay close attention to src/script/miniscript.cpp, src/script/miniscript.h, and test/fuzz/miniscript.cpp for potential logic errors and crash-prone assertions in the new miniscript implementation.
    • 




Context used:


    

  • Context from dashboard - CLAUDE.md (source)
    • 



25 files reviewed, 4 comments


Edit Code Review Agent Settings | Greptile

      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  
            
  
    
    

              

          
                test/functional/rpc_signmessagewithprivkey.py

          
        


      
          
  

    
    

          


              

              

            
              


            		
          

          


              

              

            
                  def addresses_from_privkey(self, priv_key):

            
          

          


              

              

            
                      '''Return addresses for a given WIF private key in legacy (P2PKH),

            
          

          


              

              

            
                         and script-to-hash (SH) forman.'''

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
syntax: typo: 'forman' should be 'format'


Prompt To Fix With AI

This is a comment left during a code review.
Path: test/functional/rpc_signmessagewithprivkey.py
Line: 24:24

Comment:
**syntax:** typo: 'forman' should be 'format'

How can I resolve this? If you propose a fix, please make it concise.



    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                test/get_previous_releases.py

          
        


      
          
  

    
    

          


              

              

            
                                      help='target directory.', default='releases')

            
          

          


              

              

            
                  parser.add_argument('tags', nargs='+',

            
          

          


              

              

            
                                      help="release tags. e.g.: v19.1.0 v19.0.0-rc.9")

            
          

          


              

              

            
                  all_tags = sorted([*set([v['tag'] for v in SHA256_SUMS.values()])])

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
style: sorted([*set(...)]) is redundant—set iteration order is undefined, so sorting makes sense, but the unpacking is unnecessary. Use sorted(set(...)) directly


Prompt To Fix With AI

This is a comment left during a code review.
Path: test/get_previous_releases.py
Line: 323:323

Comment:
**style:** `sorted([*set(...)])` is redundant—set iteration order is undefined, so sorting makes sense, but the unpacking is unnecessary. Use `sorted(set(...))` directly

How can I resolve this? If you propose a fix, please make it concise.



    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                src/test/fuzz/miniscript.cpp

          
        


      
          
  

    
    

          


              

              

            
              


            		
          

          


              

              

            
                  template<typename I>

            
          

          


              

              

            
                  std::optional<Key> FromPKHBytes(I first, I last) const {

            
          

          


              

              

            
                      assert(last - first == 20);

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
logic: assert assumes exactly 20 bytes, but no prior validation ensures last - first == 20. If the input is shorter or longer, this will fail in debug builds or cause undefined behavior in release builds. Does the miniscript parsing layer guarantee that FromPKHBytes is only called with exactly 20-byte inputs, or should this function validate the size and return std::nullopt instead of asserting?


Prompt To Fix With AI

This is a comment left during a code review.
Path: src/test/fuzz/miniscript.cpp
Line: 80:80

Comment:
**logic:** assert assumes exactly 20 bytes, but no prior validation ensures `last - first == 20`. If the input is shorter or longer, this will fail in debug builds or cause undefined behavior in release builds. Does the miniscript parsing layer guarantee that FromPKHBytes is only called with exactly 20-byte inputs, or should this function validate the size and return std::nullopt instead of asserting?

How can I resolve this? If you propose a fix, please make it concise.



    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                src/test/fuzz/miniscript.cpp

          
        


      
          
  

    
    

          


              

              

            
              


            		
          

          


              

              

            
                  const std::vector<unsigned char>& ToPKBytes(const Key& key) const

            
          

          


              

              

            
                  {

            
          

          


              

              

            
                      assert(!key.is_hash);

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
logic: assert without validation - if key.is_hash is true, this will fail. Should ToPKBytes either validate and return an error, or should the caller guarantee is_hash is false? Is it guaranteed by the miniscript logic that ToPKBytes is never called on a key where is_hash is true, or should this return an error instead of asserting?


Prompt To Fix With AI

This is a comment left during a code review.
Path: src/test/fuzz/miniscript.cpp
Line: 103:103

Comment:
**logic:** assert without validation - if `key.is_hash` is true, this will fail. Should ToPKBytes either validate and return an error, or should the caller guarantee `is_hash` is false? Is it guaranteed by the miniscript logic that ToPKBytes is never called on a key where `is_hash` is true, or should this return an error instead of asserting?

How can I resolve this? If you propose a fix, please make it concise.



    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  








  








      

  
        

  

        

  coderabbitai[bot]
  

  
            

              
                    coderabbitai
  bot

              

              reviewed


              
                  
                    Oct 26, 2025
                  
              
              
            


            



                
    View reviewed changes
  


            





          

  
  
  
              

                      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        @coderabbitai

      
            coderabbitai
  bot

      

      

      left a comment




      
    


  



                  

                      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
Actionable comments posted: 0



♻️ Duplicate comments (2)



src/wallet/hdchain.cpp (1)


132-138: Verify clang-format compliance.


The logic is correct - this wrapper propagates derivation failures via exceptions, consistent with the wallet's error-handling pattern. However, a previous review flagged a clang-format issue on this function signature. Please verify formatting compliance.


Run the following to verify formatting:


#!/bin/bash
# Verify clang-format compliance for this file
clang-format --dry-run -Werror src/wallet/hdchain.cpp
if [ $? -ne 0 ]; then
  echo "Formatting issues detected. Run: clang-format -i src/wallet/hdchain.cpp"
  exit 1
else
  echo "No formatting issues detected."
fi





test/functional/rpc_signmessagewithprivkey.py (1)


22-26: Fix typo in docstring.


Line 24 contains a typo: "forman" should be "format".


Apply this diff:


-        '''Return addresses for a given WIF private key in legacy (P2PKH),
-           and script-to-hash (SH) forman.'''
+        '''Return addresses for a given WIF private key in legacy (P2PKH),
+           and script-to-hash (SH) format.'''







🧹 Nitpick comments (1)



test/get_previous_releases.py (1)


323-328: Tag extraction logic is correct.


The code properly extracts unique tags from the restructured SHA256_SUMS and provides them as the default argument. Functionality is sound.


Optionally, line 323 could use a slightly more idiomatic approach:


-    all_tags = sorted([*set([v['tag'] for v in SHA256_SUMS.values()])])
+    all_tags = sorted(set(v['tag'] for v in SHA256_SUMS.values()))


This is purely a style preference and doesn't affect correctness. Since this is a backport PR, the current form may intentionally match the upstream implementation.







📜 Review details

Configuration used: CodeRabbit UI


Review profile: CHILL


Plan: Pro



📥 Commits

Reviewing files that changed from the base of the PR and between e4ce074 and adee584.





📒 Files selected for processing (12)

    

  • src/key.cpp (1 hunks)
    • 

  • src/key.h (2 hunks)
    • 

  • src/outputtype.cpp (1 hunks)
    • 

  • src/outputtype.h (1 hunks)
    • 

  • src/pubkey.cpp (1 hunks)
    • 

  • src/pubkey.h (2 hunks)
    • 

  • src/script/descriptor.cpp (3 hunks)
    • 

  • src/test/bip32_tests.cpp (1 hunks)
    • 

  • src/test/descriptor_tests.cpp (2 hunks)
    • 

  • src/wallet/hdchain.cpp (2 hunks)
    • 

  • test/functional/rpc_signmessagewithprivkey.py (2 hunks)
    • 

  • test/get_previous_releases.py (3 hunks)
    • 






🚧 Files skipped from review as they are similar to previous changes (6)

    

  • src/outputtype.h
    • 

  • src/outputtype.cpp
    • 

  • src/key.h
    • 

  • src/test/descriptor_tests.cpp
    • 

  • src/key.cpp
    • 

  • src/test/bip32_tests.cpp
    • 






🧰 Additional context used


📓 Path-based instructions (2)


src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1

Dash uses unordered_lru_cache for efficient caching with LRU eviction




Files:


    

  • src/script/descriptor.cpp
    • 

  • src/pubkey.h
    • 

  • src/wallet/hdchain.cpp
    • 

  • src/pubkey.cpp
    • 






test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)




Functional tests should be written in Python and placed in test/functional/




Files:


    

  • test/functional/rpc_signmessagewithprivkey.py
    • 






🧠 Learnings (1)


📓 Common learnings

Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.



Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.



Learnt from: knst
PR: dashpay/dash#6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.






🧬 Code graph analysis (2)


src/pubkey.h (2)



src/key.cpp (4)

    

  • Derive (287-305)
    • 

  • Derive (287-287)
    • 

  • Derive (343-350)
    • 

  • Derive (343-343)
    • 






src/pubkey.cpp (4)

    

  • Derive (240-259)
    • 

  • Derive (240-240)
    • 

  • Derive (311-318)
    • 

  • Derive (311-311)
    • 








test/functional/rpc_signmessagewithprivkey.py (1)



test/functional/test_framework/util.py (2)

    

  • assert_equal (69-74)
    • 

  • assert_raises_rpc_error (132-148)
    • 








🪛 Gitleaks (8.28.0)


test/functional/rpc_signmessagewithprivkey.py

[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.


(generic-api-key)









🔇 Additional comments (12)



test/get_previous_releases.py (2)


22-105: LGTM! Data structure upgrade is well-executed.


The restructuring of SHA256_SUMS from hash→string to hash→dict provides better semantic organization by explicitly pairing tags with tarballs. All 80+ entries follow the consistent structure.




170-171: Validation logic correctly adapted to new structure.


The hash validation properly extracts tarball strings using [v['tarball'] for v in SHA256_SUMS.values()] for membership checking. This correctly distinguishes between checksum mismatches (line 172) and missing versions (line 175).


Note: The past review comment from greptile-apps about line 171 "always being False" does not apply to the current implementation, which correctly uses a list comprehension to extract strings.





test/functional/rpc_signmessagewithprivkey.py (2)


7-9: LGTM!


The import is necessary for the new addresses_from_privkey helper method that creates descriptor checksums.




37-44: LGTM!


The enhanced verification logic correctly tests both the success case (P2PKH address) and failure cases (non-P2PKH addresses), improving test coverage.





src/pubkey.cpp (1)


312-312: LGTM!


The depth-bound check correctly prevents derivation at maximum depth, avoiding overflow when incrementing nDepth. This aligns with the broader PR changes adding explicit error handling to key derivation.





src/wallet/hdchain.cpp (1)


157-165: LGTM!


The derivation path implementation correctly uses the new DeriveExtKey wrapper at each step, ensuring failures propagate as exceptions throughout the BIP44 key derivation hierarchy.





src/pubkey.h (2)


211-211: LGTM!


The [[nodiscard]] attribute correctly enforces that callers must check the return value of Derive, preventing silent derivation failures.




284-284: LGTM!


The [[nodiscard]] attribute correctly enforces return value checking for extended public key derivation, consistent with the pattern applied to related key derivation methods.





src/script/descriptor.cpp (4)


307-307: LGTM!


The explicit failure check correctly propagates derivation errors to the caller instead of assuming success, aligning with the [[nodiscard]] attributes added to Derive methods.




367-367: LGTM!


The explicit derivation failure check maintains consistency across all Derive call sites in the descriptor implementation.




373-373: LGTM!


The consolidated check correctly detects derivation failure regardless of which derivation path was taken (cached, hardened, or unhardened).




476-477: LGTM!


Both derivation paths (unhardened and hardened) correctly check for failure and propagate errors, completing the pattern of explicit error handling throughout the descriptor implementation.








      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  




  








      

  
        

    

  
  

  
          fanquake
  
and others
        added 10 commits
        October 27, 2025 15:23


  

      

  
  

  
          

  

    

      


  

      
        @fanquake
      
        @knst
  




      

        
          Merge bitcoin#24860: Miniscript integration follow-ups
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          7601680
        
      

    

  

    

      
f3a50c9 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe516 miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e42 miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d50 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064d miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3 miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c66 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)

Pull request description:

  The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to bitcoin#24148 but i think there are enough of them to be worth a followup PR on its own.

  Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
  We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in bitcoin#24149 that will be contained in this file too.

  The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
  It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after bitcoin#24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.

  This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)

ACKs for top commit:
  sipa:
    utACK f3a50c9 (with the caveat that a lot of it is my own code)
  sanket1729:
    code review ACK f3a50c9. Did not review the fuzz tests.

Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb

    







      

  
  

  
          

  

    

      


  

      
        @knst
  




      

        
          Merge bitcoin#25709: script: actually trigger the optimization in Bui…
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          0beeb91
        
      

    

  

    

      
…ldScript

00897d0 script: actually trigger the optimization in BuildScript (Antoine Poinsot)

Pull request description:

  The counter is an optimization over calling `ret.empty()`. It was
  suggested that the compiler would realize `cnt` is only `0` on the first
  iteration, and not actually emit the check and conditional.

  This optimization was actually not triggered at all, since we
  incremented `cnt` at the beginning of the first iteration. Fix it by
  incrementing at the end instead.

  This was reported by Github user "Janus".

  Fixes bitcoin#25682. Note this does *not* change semantics. It only allows the optimization of moving instead of copying on first `CScript` element to actually be reachable.

ACKs for top commit:
  sipa:
    utACK 00897d0
  MarcoFalke:
    review ACK 00897d0

Tree-SHA512: b575bd444b0cd2fe754ec5f3e2f3f53d2696d5dcebedcace1e38be372c82365e75938dfe185429ed5a83efe1a395e204bfb33efe56c10defc5811eaee50580e3

    







      

  
  

  
          

  

    

      


  

      
        @fanquake
      
        @knst
  




      

        
          Merge bitcoin#25829: build: include share/rpcauth in release tarball
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          f658def
        
      

    

  

    

      
495caa9 build: add example bitcoin conf to win installer (fanquake)
0eac1f7 build: include share/rpcauth in tarball & installer (fanquake)

Pull request description:

  Fixes bitcoin#19081.
  Addresses: bitcoin#22235 (comment).

  Guix Build (x86_64):
  ```bash
  bdc14efe02553703f3f27438b17ee452fc0b08e37f626830857c60a7740ec990  guix-build-495caa9a0149/output/aarch64-linux-gnu/SHA256SUMS.part
  0c541efd8e2e52f1915cab13c935f165f906e46590175d568be34e026b5f0d9e  guix-build-495caa9a0149/output/aarch64-linux-gnu/bitcoin-495caa9a0149-aarch64-linux-gnu-debug.tar.gz
  5ee15e11e0d52d5a2df6b6a60e4ae5b3adc4b259294ad53452478f28efbe70c8  guix-build-495caa9a0149/output/aarch64-linux-gnu/bitcoin-495caa9a0149-aarch64-linux-gnu.tar.gz
  451152a282fd79db45a09e0ccb3491e3044ea5ad0931dfe2820bb5c45839a311  guix-build-495caa9a0149/output/arm-linux-gnueabihf/SHA256SUMS.part
  b88159ac78d427556e7f70e5da3b849da5491c98fa08c16c25adbfa8e3b44b58  guix-build-495caa9a0149/output/arm-linux-gnueabihf/bitcoin-495caa9a0149-arm-linux-gnueabihf-debug.tar.gz
  54b59e4d06144d18b97a0260330383dafdcf3473b560c71f6304a8d1b9ef0100  guix-build-495caa9a0149/output/arm-linux-gnueabihf/bitcoin-495caa9a0149-arm-linux-gnueabihf.tar.gz
  784f43a651ed6f9335117ca971c7c215eba536414792edf72a0dc372c901c3fa  guix-build-495caa9a0149/output/arm64-apple-darwin/SHA256SUMS.part
  ace525b20dc4789c931cf781027680e5fe35bd987c8d1908590cd83ce7e44495  guix-build-495caa9a0149/output/arm64-apple-darwin/bitcoin-495caa9a0149-arm64-apple-darwin-unsigned.dmg
  1ab6d5d91040567e4fccb4e2de49789f78bd1ce9b4a496486d837904591bfbc6  guix-build-495caa9a0149/output/arm64-apple-darwin/bitcoin-495caa9a0149-arm64-apple-darwin-unsigned.tar.gz
  a5cd681e67beda05cac83d133d6996699effb12b9830b269669057d02b0a4eef  guix-build-495caa9a0149/output/arm64-apple-darwin/bitcoin-495caa9a0149-arm64-apple-darwin.tar.gz
  01cc3a078a952ed9bbff38e79bad4885a5f8502906498f7b909cb2cc798ada64  guix-build-495caa9a0149/output/dist-archive/bitcoin-495caa9a0149.tar.gz
  d4652f912573d0c9ce11d7e84fad10ad03f67247f85b0a5d9c47db74508705ce  guix-build-495caa9a0149/output/powerpc64-linux-gnu/SHA256SUMS.part
  76facbb81819b2d81afd61e2234b65d99c9e28b28f2b1d3ee2d7119cf192ba60  guix-build-495caa9a0149/output/powerpc64-linux-gnu/bitcoin-495caa9a0149-powerpc64-linux-gnu-debug.tar.gz
  a75cef05abb05da4a1ba81669f6a66ecc790946b6b2fd3749bad0b462bb50219  guix-build-495caa9a0149/output/powerpc64-linux-gnu/bitcoin-495caa9a0149-powerpc64-linux-gnu.tar.gz
  2b2ac79848a643f025960cabca2694cb3537d72d83477ed1b53bcc6e2ee11ce4  guix-build-495caa9a0149/output/powerpc64le-linux-gnu/SHA256SUMS.part
  d0542623109fb72d72dcd06f8de30edefdd7e0f1bf46a7be7c262c0e631c49d8  guix-build-495caa9a0149/output/powerpc64le-linux-gnu/bitcoin-495caa9a0149-powerpc64le-linux-gnu-debug.tar.gz
  b85dbcbe4f50cbddcd167568d31326d62e786720cb237aaf3d11b69394f01983  guix-build-495caa9a0149/output/powerpc64le-linux-gnu/bitcoin-495caa9a0149-powerpc64le-linux-gnu.tar.gz
  a85a00d353338d2e72a4218782e05a816f21aea0637eb21742c55ee9a9ce1f7b  guix-build-495caa9a0149/output/riscv64-linux-gnu/SHA256SUMS.part
  e10561b08a91a22947ead27b6d4a07581fa566eb3e5d8161f97b2ff2708b85f4  guix-build-495caa9a0149/output/riscv64-linux-gnu/bitcoin-495caa9a0149-riscv64-linux-gnu-debug.tar.gz
  994aca800c2580349a436fb1fa8195cd943363dee5c3ee7c91d726cc95cd3068  guix-build-495caa9a0149/output/riscv64-linux-gnu/bitcoin-495caa9a0149-riscv64-linux-gnu.tar.gz
  5a063e6dbf64aca4a639fea10fcff2f7a533819607558cec25a9a88bc575c0dc  guix-build-495caa9a0149/output/x86_64-apple-darwin/SHA256SUMS.part
  e7f36749c3a48ce7173ee100abb275eb217eee2713d52d8b576f8c749e0c2e9e  guix-build-495caa9a0149/output/x86_64-apple-darwin/bitcoin-495caa9a0149-x86_64-apple-darwin-unsigned.dmg
  b0289c6caba5e092d8e287c9cb4f328844bce04fce74de91e76821423b7b8153  guix-build-495caa9a0149/output/x86_64-apple-darwin/bitcoin-495caa9a0149-x86_64-apple-darwin-unsigned.tar.gz
  2bf983abcfd0d782632ef7fb116ba169a0b282264774c032a50beca016468ed0  guix-build-495caa9a0149/output/x86_64-apple-darwin/bitcoin-495caa9a0149-x86_64-apple-darwin.tar.gz
  72705350ab34f8d33ef98e5563f36aa82f15e7b450e3914f591b9c88e8b385da  guix-build-495caa9a0149/output/x86_64-linux-gnu/SHA256SUMS.part
  7b5285bfd28acf8ae1471807962d31857afe70e13ae087a8aec83d5a8ac1a184  guix-build-495caa9a0149/output/x86_64-linux-gnu/bitcoin-495caa9a0149-x86_64-linux-gnu-debug.tar.gz
  6544591754607792b731fe630bf8235e78e08c7edb4d1d210b35d6328240acc1  guix-build-495caa9a0149/output/x86_64-linux-gnu/bitcoin-495caa9a0149-x86_64-linux-gnu.tar.gz
  bbd6490489ddfde06b6cde602d317d84c09495de3cf033016ea0ba5e60770fdb  guix-build-495caa9a0149/output/x86_64-w64-mingw32/SHA256SUMS.part
  f48f6efcf5822517a697d990eddbc03011b08c5e4be4886e25ab65c8a0951aaa  guix-build-495caa9a0149/output/x86_64-w64-mingw32/bitcoin-495caa9a0149-win64-debug.zip
  b1a83e58d7b316c4743ae16d552f615aa710a4985ded5834d1de6fe6437ca3b2  guix-build-495caa9a0149/output/x86_64-w64-mingw32/bitcoin-495caa9a0149-win64-setup-unsigned.exe
  0196abf8cb27f3cb03b4688a85a97c26975db4938f7a2dc0a09f11847f6530bb  guix-build-495caa9a0149/output/x86_64-w64-mingw32/bitcoin-495caa9a0149-win64-unsigned.tar.gz
  426994640e85f82b3d3bee25f07930f952edf4a60ecd7ba41f83c82cbbcc63d1  guix-build-495caa9a0149/output/x86_64-w64-mingw32/bitcoin-495caa9a0149-win64.zip
  ```

  Guix Build (arm64):
  ```bash
  506a2fade794398ec38cf9c59ca0b454fdcbb0080d7e40f1ace698a7b08eef77  guix-build-cf39913e5748/output/arm-linux-gnueabihf/SHA256SUMS.part
  8ec3686ba80709eb024ea02b7b493e70af82383d814b105f157c94a86cb7efd6  guix-build-cf39913e5748/output/arm-linux-gnueabihf/bitcoin-cf39913e5748-arm-linux-gnueabihf-debug.tar.gz
  4d9b3456c974f7f47dca64bcc994fa04888bf08518c902d12366798829a9100d  guix-build-cf39913e5748/output/arm-linux-gnueabihf/bitcoin-cf39913e5748-arm-linux-gnueabihf.tar.gz
  7c74610dcaa1dbb7fcadb2ad59520fdfa3fa02f26ceb88ada178ac35a0644840  guix-build-cf39913e5748/output/arm64-apple-darwin/SHA256SUMS.part
  fca59c367d2b5891186c546bd87ed66dcc4f0b11433b54c49ad72aa5e0e49978  guix-build-cf39913e5748/output/arm64-apple-darwin/bitcoin-cf39913e5748-arm64-apple-darwin-unsigned.dmg
  467e5c2eeaccac995d14b41f5418ba783b31ee7c63d76d5fce5e667bd6ef6683  guix-build-cf39913e5748/output/arm64-apple-darwin/bitcoin-cf39913e5748-arm64-apple-darwin-unsigned.tar.gz
  1f19caa059cd6b4adcc089f90ce33fe7f679b3a4beb970b3ef5c3292fe5841ee  guix-build-cf39913e5748/output/arm64-apple-darwin/bitcoin-cf39913e5748-arm64-apple-darwin.tar.gz
  ea07945bf21cbe306e8afc815ccb5f4a363fb7335239fe35b3668bdef43f735a  guix-build-cf39913e5748/output/dist-archive/bitcoin-cf39913e5748.tar.gz
  b699ae65b899192ad5afbe6a069d2113fda92b833f7db31a1487e7c4c04440f5  guix-build-cf39913e5748/output/powerpc64-linux-gnu/SHA256SUMS.part
  eeced64cd3e49ecd95ec29623f7cd3afe66ceffb8296af646ee4e1f3fe658ab0  guix-build-cf39913e5748/output/powerpc64-linux-gnu/bitcoin-cf39913e5748-powerpc64-linux-gnu-debug.tar.gz
  20181015752a5d7bfc0d173fefb554b785ca0e566ffef443da6673d6a63ef10d  guix-build-cf39913e5748/output/powerpc64-linux-gnu/bitcoin-cf39913e5748-powerpc64-linux-gnu.tar.gz
  59338187f551f1c043e80f63c897899365fca4872b4d793e71423c1aa7ff1358  guix-build-cf39913e5748/output/powerpc64le-linux-gnu/SHA256SUMS.part
  33d5d3113f176da881df06f5121d6003ae7f4e2d8124da805db1edab39161e81  guix-build-cf39913e5748/output/powerpc64le-linux-gnu/bitcoin-cf39913e5748-powerpc64le-linux-gnu-debug.tar.gz
  b5e9d3bf3e02aa6e3419c9f35118cb71323e0e01280b907e48fc335f8981cb6d  guix-build-cf39913e5748/output/powerpc64le-linux-gnu/bitcoin-cf39913e5748-powerpc64le-linux-gnu.tar.gz
  ef5d9f2449ad570221209194af4c4824fc70cfacbaa020da24b9186212b81c5f  guix-build-cf39913e5748/output/riscv64-linux-gnu/SHA256SUMS.part
  aafee42808b8bb2cdccbd919470bee2a1c9fcbb05daea32d15aa56288db7fc09  guix-build-cf39913e5748/output/riscv64-linux-gnu/bitcoin-cf39913e5748-riscv64-linux-gnu-debug.tar.gz
  72e2f3c9a48f8f3f14ecac2e6a8645253a32bfad8a857d13824aedd1796cad6f  guix-build-cf39913e5748/output/riscv64-linux-gnu/bitcoin-cf39913e5748-riscv64-linux-gnu.tar.gz
  8637b4e85eb2d7be803e813fbf15ee597aa35c9910b4b413d657fa69c4d4569d  guix-build-cf39913e5748/output/x86_64-apple-darwin/SHA256SUMS.part
  7b384d2734bfd650f3f8096aa32b2aa6bddfee137c3e27ac75736989f7e6740e  guix-build-cf39913e5748/output/x86_64-apple-darwin/bitcoin-cf39913e5748-x86_64-apple-darwin-unsigned.dmg
  06960637c9322c3b97bb58331b3282e770b02b7b4d95100866a96c31400a9bb9  guix-build-cf39913e5748/output/x86_64-apple-darwin/bitcoin-cf39913e5748-x86_64-apple-darwin-unsigned.tar.gz
  cc524e73d9441fc0e396c6ae5847d9b395d27f8bfe65703520792ea509191e2e  guix-build-cf39913e5748/output/x86_64-apple-darwin/bitcoin-cf39913e5748-x86_64-apple-darwin.tar.gz
  d99a7a48aa3a226d7f87c26dd4d6f12109e71fcb9f41052c0bcf5ba4bad1c408  guix-build-cf39913e5748/output/x86_64-linux-gnu/SHA256SUMS.part
  f2598fedf273244499f9481253f5114db2e299e132f5378c312f684d0a190f5f  guix-build-cf39913e5748/output/x86_64-linux-gnu/bitcoin-cf39913e5748-x86_64-linux-gnu-debug.tar.gz
  2151744dae55c460ad0c919101fcb845872060eef957d3ae0672f12098fa87b0  guix-build-cf39913e5748/output/x86_64-linux-gnu/bitcoin-cf39913e5748-x86_64-linux-gnu.tar.gz
  6087d880cced5f30d9b70b592c91efc5677f470f82cc9ed00e0fc112ff36ac43  guix-build-cf39913e5748/output/x86_64-w64-mingw32/SHA256SUMS.part
  e91e48edb8819cd25b6d6b5f5421473c56859dbb17cf32108e906877097d454d  guix-build-cf39913e5748/output/x86_64-w64-mingw32/bitcoin-cf39913e5748-win64-debug.zip
  ee4987cb94dcf4b5dfee65341a7a4f7b0865f1466c715c033a05591a749d43c3  guix-build-cf39913e5748/output/x86_64-w64-mingw32/bitcoin-cf39913e5748-win64-setup-unsigned.exe
  43001b416664ea227d3f07100d8a60bcd20c761770a332374a1466d7af734127  guix-build-cf39913e5748/output/x86_64-w64-mingw32/bitcoin-cf39913e5748-win64-unsigned.tar.gz
  bb9a56856800e7d6e5c602302ec90704cb5dd917f52d122d783deee3ab1f149d  guix-build-cf39913e5748/output/x86_64-w64-mingw32/bitcoin-cf39913e5748-win64.zip
  ```

ACKs for top commit:
  hebasto:
    ACK 495caa9, tested `bitcoin-495caa9a0149-win64-setup-unsigned.exe` on Windows 11 Pro:
  josibake:
    ACK bitcoin@495caa9

Tree-SHA512: aca9101aec9b517defc915c213af7c2fec0c254c13374e13bff715e1ee771a20fc1b487457d48809af0b82321c0be8c23c5e02444fadfca53bc6bfe16c0f4239

    







      

  
  

  
          

  

    

      


  

      
        @fanquake
      
        @knst
  




      

        
          Merge bitcoin#23703: scripted-diff: Use named args in RPC docs
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          2227faa
        
      

    

  

    

      
fa9aaf8 scripted-diff: Use named args in RPC docs (MarcoFalke)

Pull request description:

  Incorrect named args are source of bugs, like bitcoin#22979.

  To allow them being checked by `clang-tidy`, use a format it can understand.

ACKs for top commit:
  fanquake:
    ACK fa9aaf8 - checked `clang-tidy` and it's fine here, (but throwing errors in other files. i.e `wallet/test/wallet_tests.cpp`).

Tree-SHA512: e09dae8ee999a5c4819e6f848c12139593ca0e915e645c8fabeb97c379188fb9104d286c02c71f590abc64cdec125f78026735f83e016111976baa49d588a9bc

    







      

  
  

  
          

  

    

      


  

      
        @knst
  




      

        
          Merge bitcoin#25650: script: default to necessary tags in `test/get_p…
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          7e3cc68
        
      

    

  

    

      
…revious_releases.py`

BACKPORT NOTE:
removed also sources from list of hashes as they have no usage

21a9e94 ci: remove hardcoded tag list from ci scripts (josibake)
d530ba3 doc: update test/README.md (josibake)
614d468 script: default to necessary tags in get_previous_releases.py (josibake)

Pull request description:

  Almost every time I need to use this script, I forget the tag list is needed and that a specific set of tags is needed for the backwards compatibility tests to work. I end up wasting time reading through the script and googling to find the tag list before remembering it is in `test/README.md`

  I assume (hope) I'm not the only one this happens to, so I figured it would make more sense to have the script default to downloading/building the necessary tags. This has the added benefit of making the script the source of truth: the script already needs to be updated with the SHA256_SUM of the binary for every new tag that is added, so it makes sense to use `SHA256_SUMS` list as the necessary tag list. This means there is less risk of the README and the script drifting (i.e updating the readme with a new tag and forgetting to update the script, or updating the script and forgetting to update the README). Now all that needs to happen is to update the `SHA256_SUMS` list in the script and everything Just Works (TM)

ACKs for top commit:
  Sjors:
    re-tACK 21a9e94

Tree-SHA512: 97b488227a89a6827584edd251820a7074fad75dfd7f26f1aa5f858e2521d2e02effd0f11e6dc4676e1155d3d5aba6ff94a4b58ffef80dc201376afd5927deb9

    







      

  
  

  
          

  

    

      


  

      
        @knst
  




      

        
          Merge bitcoin#25760: rest: clean-up for mempool endpoints
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          4084093
        
      

    

  

    

      
acbea66 rest: clean-up for `mempool` endpoints (brunoerg)

Pull request description:

  The functions `rest_mempool_info` and `rest_mempool_contents` are similar, the only difference between them is:
  `rest_mempool_info` uses `MempoolInfoToJSON` to get the mempool informations and `rest_mempool_contents` uses `MempoolToJSON`, for this reason this PR creates a new function to handle it and reduce duplicated code.

  Also,
  1. Rename `strURIPart` to `str_uri_part`.
  2. Rename `strJSON` to `str_json`.

ACKs for top commit:
  stickies-v:
    re-ACK acbea66 - verified that just the error message was updated since bitcoin@da0c612
  theStack:
    re-ACK acbea66

Tree-SHA512: 35f6f0732a573fe8a6cdcc782f89ae3427a1de19f069a68c9c51bb525118c2b07e20303cbe19b9d4b7d1ad055d69c32def2d0fb8f886c851da562dd9ce33ad6a

    







      

  
  

  
          

  

    

      


  

      
        @achow101
      
        @knst
  




      

        
          Merge bitcoin#25642: Don't wrap around when deriving an extended key …
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          5f7652d
        
      

    

  

    

      
…at a too large depth

fb9faff extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670 descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c2 spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot)

Pull request description:

  We would previously  silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.

  An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke.

  Fixes bitcoin#25751.

ACKs for top commit:
  achow101:
    re-ACK fb9faff
  instagibbs:
    utACK  bitcoin@fb9faff

Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343

    







      

  
  

  
          

  

    

      


  

      
        @achow101
      
        @knst
  




      

        
          Merge bitcoin#25869: wallet: remove UNKNOWN type from OUTPUT_TYPES array
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          f32a578
        
      

    

  

    

      
5b4fdbb wallet: remove UNKNOWN type from OUTPUT_TYPES array (furszy)

Pull request description:

  Fixing bitcoin#25734 (comment) ->  https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=50329

  The `OUTPUT_TYPES` array contain the known active output types only.
  And it's solely used to create/walk-through the active spkms.

  So, no need to add the `UNKNOWN` type here.

ACKs for top commit:
  achow101:
    ACK 5b4fdbb
  w0xlt:
    ACK bitcoin@5b4fdbb
  LarryRuane:
    ACK 5b4fdbb

Tree-SHA512: dee2dc362a1b0c777555e5ee4d355a3351340591d0096f74e8c3a25f374cb2d9aef26145977ff4dd0f8cc940da9464eb5541eb2895bc19f8cbd6bb6d292ab9a9

    







      

  
  

  
          

  

    

      


  

      
        @achow101
      
        @knst
  




      

        
          Merge bitcoin#25782: test: check that verifymessage RPC fails for n…
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          561f650
        
      

    

  

    

      
…on-P2PKH addresses

68006c1 test: check that `verifymessage` RPC fails for non-P2PKH addresses (Sebastian Falbesoner)

Pull request description:

  This PR adds missing test coverage for the `verifymessage` RPC, for the case that a non-P2PKH (but otherwise valid) address is passed:
  https://github.com/bitcoin/bitcoin/blob/e09ad284c762a79d59417389e9056c18e25d9770/src/util/message.cpp#L38-L40
  https://github.com/bitcoin/bitcoin/blob/e09ad284c762a79d59417389e9056c18e25d9770/src/rpc/signmessage.cpp#L48-L49
  The passed addresses to trigger the error are of the types nested segwit (P2SH-P2WPKH) and native segwit (P2WPKH) and are created with a helper function `addresses_from_privkey` using descriptors and the `deriveaddresses` RPC. At some point in the future, if we have BIP322 support, all those will likely succeed and can then be moved from error-throwing to the succedding assert loop.

ACKs for top commit:
  achow101:
    ACK 68006c1
  w0xlt:
    ACK bitcoin@68006c1

Tree-SHA512: fec4ed97460787c2ef3d04e3fce89c9365c87207c8358b59c41890f3738355c002e64f289ab4aef794ef4dfd5c867be8b67d736fb620489204f2c6bfb8d3363c

    







      

  
  

  
          

  

    

      


  

      
        @knst
  




      

        
          Merge bitcoin#26589: test: small fixups/improvements for get_previous…
        

          
                        
      


      

        

    
    
    

  

    


      


      

        

          

    
    
    

  

  

    
  



        

      


      

        
          fd7ca49
        
      

    

  

    

      
…_releases.py

9b5feb7 script: small fixups/improvements for get_previous_releases.py (Sebastian Falbesoner)

Pull request description:

  This is a small follow-up to bitcoin#25650 (commit 614d468) with three fixes/improvements:

  - fix "Checksum did not match" detection, which was not adapted to the new `SHA256_SUMS` structure and hence never executed (the list of tarball names isn't directly in the dictionary's values anymore, but has to be extracted from the `'tarball'` field of each value)
  - make both help text and default tag download order deterministic by sorting default tags
  - `--tags` argument help text: add missing space between "for" and "backwards"

ACKs for top commit:
  Sjors:
    tACK 9b5feb7. Tested that if I change a checksum, or remove a release, it catches that.
  josibake:
    tested ACK bitcoin@9b5feb7

Tree-SHA512: 791fa693477eebbda7fd41f3f5ec78fe7eab57df06979aa907ab258a6945534bdc3b931ddfce0fb440c9666b98c88ce5e1b6dc353ed39e129e87d3634855165c

    







  








      

  
          







        

  
        

  

        

  coderabbitai[bot]
  

  
            

              
                    coderabbitai
  bot

              

              reviewed


              
                  
                    Oct 27, 2025
                  
              
              
            


            



                
    View reviewed changes
  


            





          

  
  
  
              

                      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        @coderabbitai

      
            coderabbitai
  bot

      

      

      left a comment




      
    


  



                  

                      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
Actionable comments posted: 1


Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.



⚠️ Outside diff range comments (1)



src/script/miniscript.h (1)


8-14: Add missing header for std::set.


Node::ContainsDuplicateKey uses std::set, but  isn’t included here. This will fail to compile on some toolchains.


Apply this diff:


 #include <algorithm>
-#include <functional>
+#include <functional>
+#include <set>
 #include <numeric>









♻️ Duplicate comments (3)



test/functional/rpc_signmessagewithprivkey.py (1)


22-26: Typo in docstring (already flagged).


Line 24 contains "forman" which should be "format". This has already been identified by previous reviewers.


Note: Since this is a backport PR, if this typo exists in the upstream Bitcoin Core code, it should be kept as-is to maintain fidelity with the original and reduce merge conflicts in future backports. Based on learnings.





test/get_previous_releases.py (1)


323-323: Simplify redundant unpacking in sorted().


The list unpacking [*set(...)] is unnecessary since sorted() accepts any iterable directly.


Apply this diff:


-    all_tags = sorted([*set([v['tag'] for v in SHA256_SUMS.values()])])
+    all_tags = sorted({v['tag'] for v in SHA256_SUMS.values()})





src/test/bip32_tests.cpp (1)


187-204: Max-depth test covers both xprv/xpub paths—LGTM


Semantics are right and align with new Derive behavior. Minor: the loop style for (auto i = 0; i++ < 255;) is unusual; for (int i = 0; i < 255; ++i) is clearer. Defer change if mirroring upstream.


-    for (auto i = 0; i++ < 255;) {
+    for (int i = 0; i < 255; ++i) {


Based on learnings







🧹 Nitpick comments (2)



test/README.md (1)


105-107: Consider specifying shell language for the fenced code block.


The fenced code block on line 105 should specify shell or bash as the language identifier for better syntax highlighting and consistency with other code blocks in the documentation.


Apply this diff:


-```
+```shell
 test/get_previous_releases.py -b



Based on static analysis hints.

</blockquote></details>
<details>
<summary>src/pubkey.cpp (1)</summary><blockquote>

`311-318`: **Depth guard at 255 is correct; consider explicit <limits> include**

Early-return on max unsigned char depth prevents overflow; matches private-key path and tests. Please ensure <limits> is included here to avoid transitive include reliance.   

```diff
 #include <algorithm>
 #include <cassert>
+#include <limits>



Based on learnings







📜 Review details

Configuration used: CodeRabbit UI


Review profile: CHILL


Plan: Pro



📥 Commits

Reviewing files that changed from the base of the PR and between adee584 and fd7ca49.





📒 Files selected for processing (26)

    

  • ci/test/00_setup_env_native_qt5.sh (1 hunks)
    • 

  • ci/test/05_before_script.sh (1 hunks)
    • 

  • contrib/guix/libexec/build.sh (1 hunks)
    • 

  • share/setup.nsi.in (2 hunks)
    • 

  • src/Makefile.test.include (1 hunks)
    • 

  • src/key.cpp (1 hunks)
    • 

  • src/key.h (2 hunks)
    • 

  • src/outputtype.cpp (1 hunks)
    • 

  • src/outputtype.h (1 hunks)
    • 

  • src/pubkey.cpp (1 hunks)
    • 

  • src/pubkey.h (2 hunks)
    • 

  • src/rest.cpp (2 hunks)
    • 

  • src/rpc/output_script.cpp (1 hunks)
    • 

  • src/script/descriptor.cpp (3 hunks)
    • 

  • src/script/miniscript.cpp (5 hunks)
    • 

  • src/script/miniscript.h (33 hunks)
    • 

  • src/script/script.h (1 hunks)
    • 

  • src/test/bip32_tests.cpp (1 hunks)
    • 

  • src/test/descriptor_tests.cpp (2 hunks)
    • 

  • src/test/fuzz/miniscript.cpp (1 hunks)
    • 

  • src/test/fuzz/miniscript_decode.cpp (0 hunks)
    • 

  • src/test/miniscript_tests.cpp (3 hunks)
    • 

  • src/wallet/hdchain.cpp (2 hunks)
    • 

  • test/README.md (1 hunks)
    • 

  • test/functional/rpc_signmessagewithprivkey.py (2 hunks)
    • 

  • test/get_previous_releases.py (3 hunks)
    • 






💤 Files with no reviewable changes (1)

    

  • src/test/fuzz/miniscript_decode.cpp
    • 






✅ Files skipped from review due to trivial changes (1)

    

  • src/rpc/output_script.cpp
    • 






🚧 Files skipped from review as they are similar to previous changes (9)

    

  • src/Makefile.test.include
    • 

  • src/outputtype.h
    • 

  • src/wallet/hdchain.cpp
    • 

  • src/test/descriptor_tests.cpp
    • 

  • ci/test/05_before_script.sh
    • 

  • src/key.cpp
    • 

  • src/script/script.h
    • 

  • contrib/guix/libexec/build.sh
    • 

  • src/test/fuzz/miniscript.cpp
    • 






🧰 Additional context used


📓 Path-based instructions (4)


src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1

Dash uses unordered_lru_cache for efficient caching with LRU eviction




Files:


    

  • src/test/bip32_tests.cpp
    • 

  • src/pubkey.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 

  • src/key.h
    • 

  • src/pubkey.h
    • 

  • src/script/descriptor.cpp
    • 

  • src/rest.cpp
    • 

  • src/script/miniscript.cpp
    • 

  • src/outputtype.cpp
    • 

  • src/script/miniscript.h
    • 






src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)




Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests




Files:


    

  • src/test/bip32_tests.cpp
    • 

  • src/test/miniscript_tests.cpp
    • 






ci/**

📄 CodeRabbit inference engine (CLAUDE.md)




Unless specifically prompted, avoid making changes to the ci directory (continuous integration)




Files:


    

  • ci/test/00_setup_env_native_qt5.sh
    • 






test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)




Functional tests should be written in Python and placed in test/functional/




Files:


    

  • test/functional/rpc_signmessagewithprivkey.py
    • 






🧠 Learnings (1)


📓 Common learnings

Learnt from: knst
PR: dashpay/dash#6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.889Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.



Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.



Learnt from: knst
PR: dashpay/dash#6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.



Learnt from: knst
PR: dashpay/dash#6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.






🧬 Code graph analysis (7)


src/test/miniscript_tests.cpp (1)



src/test/fuzz/miniscript.cpp (12)

    

  • a (50-52)
    • 

  • a (50-50)
    • 

  • a (97-99)
    • 

  • a (97-97)
    • 

  • key (54-60)
    • 

  • key (54-54)
    • 

  • key (101-105)
    • 

  • key (101-101)
    • 

  • key (107-112)
    • 

  • key (107-107)
    • 

  • FromString (63-68)
    • 

  • FromString (63-63)
    • 








src/key.h (1)



src/key.cpp (4)

    

  • Derive (287-305)
    • 

  • Derive (287-287)
    • 

  • Derive (343-350)
    • 

  • Derive (343-343)
    • 








src/pubkey.h (1)



src/pubkey.cpp (4)

    

  • Derive (240-259)
    • 

  • Derive (240-240)
    • 

  • Derive (311-318)
    • 

  • Derive (311-311)
    • 








src/rest.cpp (1)



src/rpc/mempool.cpp (4)

    

  • MempoolToJSON (332-371)
    • 

  • MempoolToJSON (332-332)
    • 

  • MempoolInfoToJSON (682-699)
    • 

  • MempoolInfoToJSON (682-682)
    • 








src/script/miniscript.cpp (3)



src/script/script.cpp (2)

    

  • CheckMinimalPush (386-409)
    • 

  • CheckMinimalPush (386-386)
    • 






src/script/standard.h (1)

    

  • IsPushdataOp (112-115)
    • 






src/script/script.h (2)

    

  • CScriptNum (211-226)
    • 

  • CScriptNum (230-240)
    • 








test/functional/rpc_signmessagewithprivkey.py (1)



test/functional/test_framework/util.py (2)

    

  • assert_equal (69-74)
    • 

  • assert_raises_rpc_error (132-148)
    • 








src/script/miniscript.h (2)



src/script/miniscript.cpp (8)

    

  • ComputeType (36-248)
    • 

  • ComputeType (36-36)
    • 

  • ComputeScriptLen (250-280)
    • 

  • ComputeScriptLen (250-250)
    • 

  • DecomposeScript (282-316)
    • 

  • DecomposeScript (282-282)
    • 

  • ParseScriptNumber (318-329)
    • 

  • ParseScriptNumber (318-318)
    • 






src/test/fuzz/miniscript.cpp (10)

    

  • a (50-52)
    • 

  • a (50-50)
    • 

  • a (97-99)
    • 

  • a (97-97)
    • 

  • key (54-60)
    • 

  • key (54-54)
    • 

  • key (101-105)
    • 

  • key (101-101)
    • 

  • key (107-112)
    • 

  • key (107-107)
    • 








🪛 Gitleaks (8.28.0)


test/functional/rpc_signmessagewithprivkey.py

[high] 32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.


(generic-api-key)







🪛 markdownlint-cli2 (0.18.1)


test/README.md

105-105: Fenced code blocks should have a language specified


(MD040, fenced-code-language)









🔇 Additional comments (26)



test/functional/rpc_signmessagewithprivkey.py (3)


7-9: LGTM - Import addition supports descriptor-based address derivation.


The import of descsum_create is necessary for the new descriptor-based address derivation logic in the helper method.




37-44: LGTM - Enhanced test coverage validates both success and error cases.


The test properly validates:


    

  • P2PKH address derivation and verification succeeds (lines 37-40)
    • 

  • Non-P2PKH addresses (SH) correctly fail with error code -3 and message "Address does not refer to key" (lines 42-44)
    • 



This provides good coverage for the descriptor-based address handling.




32-32: Static analysis false positive - test private key.


The static analysis tool flagged this line as containing a "Generic API Key", but this is a false positive. This is a WIF-encoded test private key used for deterministic functional testing, not a real secret.





share/setup.nsi.in (2)


133-134: LGTM!


The uninstaller correctly removes the files and directories added by the installer. The recursive removal of the share directory is appropriate since the installer creates the share\rpcauth subdirectory structure.




78-80: No issues found with the wildcard pattern.


The share/rpcauth directory contains only files with extensions (README.md and rpcauth.py), so the *.* pattern on line 80 will correctly match and copy all required files to the installer.





src/outputtype.cpp (1)


21-21: ✓ LGTM: OUTPUT_TYPES correctly updated.


Verification confirms no Dash-specific code depends on UNKNOWN being in the OUTPUT_TYPES array. All UNKNOWN references remain internal to outputtype.cpp and are properly handled with assertions. The change safely narrows the public API to expose only LEGACY as a user-selectable output type.





src/rest.cpp (2)


623-657: LGTM! Mempool endpoints correctly consolidated into unified handler.


The refactoring consolidates separate rest_mempool_info and rest_mempool_contents endpoints into a single handler with sub-path routing. The implementation correctly:


    

  • Validates sub-paths are either "contents" or "info" (lines 630-632)
    • 

  • Dispatches to appropriate JSON builders based on sub-path (lines 643-646)
    • 

  • Maintains existing warmup checks and error handling
    • 

  • Uses consistent patterns with other REST handlers in the file
    • 



Regarding the past review comment about format validation: The .json format suffix is required. While line 630 validates the path component, format validation occurs in the switch statement (line 637). Requests without .json (e.g., /rest/mempool/info) will have rf=UNDEF and fall to the default case at line 653, returning "output format not found". This two-stage validation is consistent with other REST handlers in the file.




968-968: LGTM! Dispatch table correctly updated.


The URI prefix updated to /rest/mempool/ (with trailing slash) properly routes the new unified handler for sub-paths like /rest/mempool/info.json and /rest/mempool/contents.json.





test/get_previous_releases.py (2)


22-105: SHA256_SUMS restructuring applied consistently.


The data structure change from hash: tarball_string to hash: {tag, tarball} is applied consistently across all entries. The validation logic (lines 170-171) and tag extraction (line 323) are correctly updated to match the new structure.




324-328: CLI argument change correctly implements optional tag list with default.


The argument parsing change from requiring at least one tag to accepting zero or more tags with a default full list correctly supports the new invocation pattern where test/get_previous_releases.py -b downloads all necessary releases for backward compatibility testing.





ci/test/00_setup_env_native_qt5.sh (1)


17-17: Clean transition from version list to boolean flag.


The change from PREVIOUS_RELEASES_TO_DOWNLOAD (explicit version list) to DOWNLOAD_PREVIOUS_RELEASES="true" (boolean flag) correctly aligns with the PR-wide refactoring where version selection is now handled internally by test/get_previous_releases.py via the SHA256_SUMS structure.





src/script/descriptor.cpp (1)


303-313: Good: propagate BIP32 Derive failures instead of asserting


Returning false on derivation errors (incl. cache misses/hardened paths and depth-limit) is correct and matches [[nodiscard]] semantics added elsewhere. This prevents UB and makes descriptor expansion robust.  Based on learnings


Also applies to: 366-375, 472-480





src/key.h (1)


136-141: [[nodiscard]] on Derive is a solid API hardening


Prevents accidental ignoring of failure; consistent with pubkey side and descriptor changes.  Based on learnings


Also applies to: 188-191





src/pubkey.h (1)


208-212: Consistent [[nodiscard]] annotations on public Derive APIs


Matches private-key counterparts and supports the new failure-propagation paths.  Based on learnings


Also applies to: 282-286





src/test/miniscript_tests.cpp (3)


74-76: Comparator addition is correct and minimal.


KeyCompare delegates to CPubKey::operator<, suitable for std::set comparator use in duplicate-key detection.




91-96: Good shift to optional-returning converters.


The optional-based FromString/FromPKBytes/FromPKHBytes align with the new API and avoid out-params. Assertions against g_testdata maps are fine for test-only code.


Also applies to: 99-103, 106-113




280-292: Solid coverage for duplicate-key sanity checks.


The new tests validate IsSane() and CheckDuplicateKey() across multiple structures (conjunctions, disjunctions, deep trees). Nice.





src/script/miniscript.cpp (5)


20-33: Type sanitization checks preserved.


Conflict/implication asserts match upstream intent; no issues spotted.




36-248: Fragment-centric ComputeType looks consistent.


Parameter validation and per-fragment rules mirror upstream miniscript semantics. No correctness issues found.




250-280: ComputeScriptLen switch is accurate.


Costs reflect fragment forms; WRAP_V adds verify-cost via x-property as expected.




282-316: DecomposeScript: optional result + minimal-push/VERIFY checks LGTM.


Opcode decomposition and reverse-order accumulation match DecodeScript expectations.




318-329: ParseScriptNumber: correct minimal-push and CScriptNum handling.


Returns std::nullopt on non-minimal or overflow, otherwise parsed int64. All good.





src/script/miniscript.h (4)


183-234: Public alias and helpers match the new API.


using Opcode = std::pair<opcodetype, std::vector<unsigned char>>; and the fragment-centric signatures align with miniscript.cpp changes.




304-315: New duplicate-key guarantees wired correctly.


duplicate_key cached flag and CheckDuplicateKey() expose sane/duplicate checks to callers.


Also applies to: 833-844




765-799: ContainsDuplicateKey: robust set-based merge with custom comparator.


Efficient merge logic and early-nullopt on duplicates is clean. After adding , this should be fine.




1001-1010: Wrapper construction, sanity, and FromScript glue look consistent.


BuildBack threading of ctx, top-level validity checks, and use of DecomposeScript all match the optional-based decoding flow.


Also applies to: 1041-1065, 1172-1207, 1253-1258, 1707-1714








      
    

  
  




                    

                  
  
  

    
  
    
    

  

  

  



                      

                    
                  








      

  
  
  
            
  
    
    

              

          
                src/script/miniscript.h

          
        


      
            

      

      Comment on lines

    
      +1393
     to 
      1403
    

  


  


  

    
    

          


              

              

            
                          if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && (num = ParseScriptNumber(in[1]))) {

            
          

          


              

              

            
                              in += 2;

            
          

          


              

              

            
                              if (k < 1 || k > 0x7FFFFFFFL) return {};

            
          

          


              

              

            
                              constructed.push_back(MakeNodeRef<Key>(Fragment::OLDER, k));

            
          

          


              

              

            
                              if (*num < 1 || *num > 0x7FFFFFFFL) return {};

            
          

          


              

              

            
                              constructed.push_back(MakeNodeRef<Key>(ctx, Fragment::OLDER, *num));

            
          

          


              

              

            
                              break;

            
          

          


              

              

            
                          }

            
          

          


              

              

            
                          if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && ParseScriptNumber(in[1], k)) {

            
          

          


              

              

            
                          if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {

            
          

          


              

              

            
                              in += 2;

            
          

          


              

              

            
                              if (k < 1 || k > 0x7FFFFFFFL) return {};

            
          

          


              

              

            
                              constructed.push_back(MakeNodeRef<Key>(Fragment::AFTER, k));

            
          

          


              

              

            
                              if (num < 1 || num > 0x7FFFFFFFL) return {};

            
          

          


              

              

            
                              constructed.push_back(MakeNodeRef<Key>(ctx, Fragment::AFTER, *num));

            
          

          


              

              

            
                              break;

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        @coderabbitai

      
            coderabbitai
  bot

      

      

      


        Oct 27, 2025


      
  •

  

    
      

        
          edited
          
        
        
      

    
    
      
  
        
      Loading


  
    
  



    


  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
⚠️ Potential issue | 🔴 Critical


Fix std::optional<int64_t> comparisons (compile error).


In DecodeScript, comparisons use num directly instead of *num. This won’t compile and is inconsistent with the OLDER branch.


    

  • AFTER branch lower/upper bound checks should dereference num.
    • 

  • Hash prelude check should compare *num == 32.
    • 



Apply this diff:


-            if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
+            if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
                 in += 2;
-                if (num < 1 || num > 0x7FFFFFFFL) return {};
+                if (*num < 1 || *num > 0x7FFFFFFFL) return {};
                 constructed.push_back(MakeNodeRef<Key>(ctx, Fragment::AFTER, *num));
                 break;
             }
-            if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && num == 32 && in[6].first == OP_SIZE) {
+            if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && *num == 32 && in[6].first == OP_SIZE) {


Based on learnings.


Also applies to: 1406-1413



    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    





    

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
@coderabbitai We require C++20 which should allow this afaik though it does look a bit inconsistent with other changes, I agree.

    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    





    

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
@UdjinM6, you're absolutely right that C++20 supports direct comparison of std::optional<T> with values of type T through comparison operators, so num < 1 is valid when num is std::optional<int64_t>.


However, the inconsistency here is within the same function—the OLDER branch (line ~1393) uses the dereferenced form *num, while the AFTER branch (line ~1398) uses the direct comparison form num. Both are functionally correct in C++20, but mixing styles in adjacent code can reduce readability.


For consistency within this function, I'd suggest picking one style and applying it to both branches:


    

  • Either use *num for both (more explicit), or
    • 

  • Use direct comparison num for both (more concise)
    • 



The same applies to the hash check around line 1406 if it also uses num.


Would you prefer to standardize on one style, or keep it as-is since both work?


    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  








  








        

  
        

  

        

  UdjinM6
  

  
            

              
                    UdjinM6
  

              

              reviewed


              
                  
                    Oct 27, 2025
                  
              
              
            


            



                
    View reviewed changes
  


            






      

  
  
  
            
  
    
    

              

          
                share/setup.nsi.in

          
        


      
          
  

    
    

          


              

              

            
                  File @abs_top_builddir@/release/@BITCOIN_GUI_NAME@@EXEEXT@

            
          

          


              

              

            
                  File /oname=COPYING.txt @abs_top_srcdir@/COPYING

            
          

          


              

              

            
                  File /oname=readme.txt @abs_top_srcdir@/doc/README_windows.txt

            
          

          


              

              

            
                  File @abs_top_srcdir@/share/examples/dash.conf

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
25829:

  

    

      

        Suggested change
      

    

    

      

          

            
                File @abs_top_srcdir@/share/examples/dash.conf
          

          

            
                File @abs_top_srcdir@/contrib/debian/examples/dash.conf
          

      

    

    

  


    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                test/functional/rpc_signmessagewithprivkey.py

          
        


      
          
  

    
    

          


              

              

            
              


            		
          

          


              

              

            
                  def addresses_from_privkey(self, priv_key):

            
          

          


              

              

            
                      '''Return addresses for a given WIF private key in legacy (P2PKH),

            
          

          


              

              

            
                         and script-to-hash (SH) forman.'''

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
25782:

  

    

      

        Suggested change
      

    

    

      

          

            
                       and script-to-hash (SH) forman.'''
          

          

            
                       and script-to-hash (P2SH) format.'''
          

      

    

    

  


    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


  
  
    
    

              

          
                test/get_previous_releases.py

          
        


      
          
  

    
    

          


              

              

            
                  all_tags = sorted([*set([v['tag'] for v in SHA256_SUMS.values()])])

            
          

          


              

              

            
                  parser.add_argument('tags', nargs='*', default=all_tags,

            
          

          


              

              

            
                                      help='release tags. e.g.: v19.1.0 v19.0.0-rc.9'

            
          

          


              

              

            
                                      '(if not specified, the full list needed for'

            
          

    








  

    

        

        

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

  




        

      

  

  
 

  
    

    
Choose a reason for hiding this comment


    

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


    

      
      



  



  
    

      
26589:

  

    

      

        Suggested change
      

    

    

      

          

            
                                    '(if not specified, the full list needed for'
          

          

            
                                    '(if not specified, the full list needed for '
          

      

    

    

  


    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  








  








        

  
      



  

  @UdjinM6






  



    

      

  

    

        
    
    

  


      
          
  
      
            Copy link

  

      
    

  


  

      



      

  


  

    

        

      
            UdjinM6
  

      

      

      commented


        Oct 27, 2025


      
  •

  

    
      

        
          edited
          
        
        
      

    
    
      
  
        
      Loading


  
    
  



    


  





      


        



  
    

      
          
feature_backwards_compatibility.py failed with Force test of previous releases but releases missing: https://github.com/dashpay/dash/actions/runs/18834433565/job/53735260434?pr=6916, restarted to confirm it's not some fetch issue.


EDIT: nope, failed again


      		
    

  





        


            

            
            

              
            

        

      


      

            
  
  

    
  
    
    

  

  

  


    














  
  

    

      

  




      


    


    

      

        
      

  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment


  



      

    

  




  
          



      

  

    
    

    

  

    Reviewers
  


    


        

    


  
      @UdjinM6
  
    UdjinM6



    
      
        
      
    
    UdjinM6 left review comments

  


        

    


  
      @coderabbitai
  
    coderabbitai[bot]



    
      
        
      
    
    coderabbitai[bot] left review comments

  


      

          


  
      @PastaPastaPasta
  
    PastaPastaPasta


                Awaiting requested review from PastaPastaPasta


        
      

      

          


  
      @kwvg
  
    kwvg


                Awaiting requested review from kwvg


        
      


      

        
          +1 more reviewer
          
          
        
          

              

    


  
      @greptile-apps
  
    greptile-apps[bot]



    
      
        
      
    
    greptile-apps[bot] left review comments

  


          

      

      Reviewers whose approvals may not affect merge requirements
        

      At least 2 approving reviews are required to merge this pull request.
  








    

    

  


      
  

    Assignees
  



        


    No one assigned







      


  


  

    Labels
  



    

    None yet







      

  

    

        

    Projects
  


        




    None yet




  



      


  

    
  

    Milestone
  


      
    
  
    23.1
  





    
      
          


  

    

      
        

          
  

    Development
  



            


Successfully merging this pull request may close these issues.





  
  



      
    

  




      

    

  
      

  

    

      4 participants
    

    

        
          @knst 
        
          @UdjinM6 
        
          @fanquake 
        
          @achow101