backport: bitcoin#24352, #25175, #25368, #25454, #25557, #25590, #25629, #25630

[knst]

What was done?

Regular backports from bitcoin core v24

How Has This Been Tested?

Run unit & functional tests

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

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

This pull request introduces a refactoring across multiple subsystems. Key changes include: adding AC_CONFIG_LINKS entries in configure.ac for Qt and test Makefiles; reorganizing documentation by moving assumeutxo reference and adding a new libraries design document; refactoring net_processing.cpp to change timestamp storage from atomic seconds to NodeClock::time_point and reordering lock annotations; centralizing type checking in univalue through a new checkType helper method; updating RPC help text and error messages for clarity; and updating approximately 11 test files to expect new JSON type-error message formats that explicitly state the received and expected types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

The PR spans 18+ files with mixed complexity. While many test file updates follow a repetitive pattern (error message string replacements), the core logic changes are concentrated in 3-4 key files: src/net_processing.cpp (lock ordering and timestamp type changes), src/univalue header and implementation files (type-checking refactoring), and documentation reorganization. The heterogeneity stems from combining infrastructure changes (autoconf), documentation, semantic type refactoring, and test updates, though individual change categories are relatively homogeneous. The net_processing.cpp modifications require careful attention to lock semantics and timestamp handling.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title consists solely of a list of backport PR numbers and does not describe the nature of the changes being applied, making it difficult for a reviewer or teammate to understand the primary purpose of the pull request at a glance. It lacks a concise summary of the main update.	Please rephrase the title to clearly summarize the main change, for example “Backport core v24 compatibility fixes and update type error messages,” omitting the list of PR numbers and focusing on what the backports implement.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	The pull request description clearly states that it includes regular backports from Bitcoin Core v24, notes testing by running unit and functional tests, and lists the checklist items, which aligns with the changeset and is not off-topic.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b17f8f5 and fc3e8cc.

📒 Files selected for processing (1)

• configure.ac (1 hunks)

🧰 Additional context used

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

🔇 Additional comments (1)

configure.ac (1)

2002-2004: LGTM! Standard autoconf pattern for out-of-tree builds.

The three new AC_CONFIG_LINKS entries follow the established pattern seen in lines 1995-2001 and are consistent with autoconf best practices for ensuring Makefiles are available in the build directory during out-of-tree builds. The syntax is correct, and since this is a backport from Bitcoin Core v24 with successful CI runs, the referenced Makefiles are confirmed to exist at the specified paths.

Warning

Review ran into problems

🔥 Problems

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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 (2)

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

9-18: Add missing include for std::errc

getInt() uses std::errc; include <system_error> for portability across libstdc++/libc++.

Apply:

 #include <charconv>
 #include <cstdint>
 #include <cstring>
 #include <map>
 #include <stdexcept>
+#include <system_error>
 #include <string>
 #include <string_view>
 #include <type_traits>
 #include <vector>

---

103-115: Edits in vendored dependency (src/univalue/*) conflict with guidelines

Guidelines disallow changes under src/univalue. Please confirm exception or consider updating via upstream/vendor bump to carry these changes.

As per coding guidelines

Also applies to: 128-144

🧹 Nitpick comments (3)

src/univalue/lib/univalue_get.cpp (1)

53-58: Optional: align getValues() error message with new format

Consider using a unified message format (e.g., “JSON value of type X is not of expected type object/array”) for consistency with other getters.

src/dbwrapper.cpp (1)

7-19: Minor include hygiene nit

Some LevelDB/project headers are included both here and via dbwrapper.h (e.g., leveldb/db.h). Not harmful, but consider trimming redundant includes to speed up builds and help IWYU converge.

Also applies to: 24-27

src/node/chainstate.cpp (1)

88-91: Consider scoped cleanup on error paths after setup

After DashChainstateSetup connects managers, multiple early returns below can leave components partially wired (e.g., mempool still connected). Consider a small RAII guard or calling DashChainstateSetupClose on failure paths to keep state consistent.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f170aed and b17f8f5.

📒 Files selected for processing (34)

• ci/dash/lint-tidy.sh (1 hunks)
• configure.ac (1 hunks)
• doc/README.md (1 hunks)
• doc/design/libraries.md (1 hunks)
• src/bench/checkblock.cpp (1 hunks)
• src/chain.h (1 hunks)
• src/dbwrapper.cpp (1 hunks)
• src/dbwrapper.h (1 hunks)
• src/evo/evodb.cpp (1 hunks)
• src/evo/specialtxman.cpp (1 hunks)
• src/index/base.cpp (1 hunks)
• src/index/blockfilterindex.cpp (1 hunks)
• src/index/coinstatsindex.cpp (1 hunks)
• src/net_processing.cpp (6 hunks)
• src/node/chainstate.cpp (2 hunks)
• src/qt/optionsdialog.cpp (1 hunks)
• src/rpc/fees.cpp (0 hunks)
• src/rpc/mempool.cpp (1 hunks)
• src/txdb.h (1 hunks)
• src/univalue/include/univalue.h (3 hunks)
• src/univalue/lib/univalue.cpp (3 hunks)
• src/univalue/lib/univalue_get.cpp (3 hunks)
• src/wallet/rpc/backup.cpp (1 hunks)
• src/wallet/rpc/util.cpp (2 hunks)
• test/functional/feature_minchainwork.py (1 hunks)
• test/functional/mining_prioritisetransaction.py (1 hunks)
• test/functional/rpc_blockchain.py (2 hunks)
• test/functional/rpc_help.py (1 hunks)
• test/functional/rpc_rawtransaction.py (4 hunks)
• test/functional/test_framework/test_framework.py (1 hunks)
• test/functional/wallet_basic.py (1 hunks)
• test/functional/wallet_descriptor.py (1 hunks)
• test/functional/wallet_hd.py (1 hunks)
• test/functional/wallet_multiwallet.py (1 hunks)

💤 Files with no reviewable changes (1)

• src/rpc/fees.cpp

🧰 Additional context used

📓 Path-based instructions (7)

test/functional/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

• test/functional/rpc_help.py
• test/functional/wallet_basic.py
• test/functional/feature_minchainwork.py
• test/functional/wallet_multiwallet.py
• test/functional/wallet_descriptor.py
• test/functional/mining_prioritisetransaction.py
• test/functional/rpc_rawtransaction.py
• test/functional/test_framework/test_framework.py
• test/functional/wallet_hd.py
• test/functional/rpc_blockchain.py

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/index/base.cpp
• src/chain.h
• src/evo/specialtxman.cpp
• src/evo/evodb.cpp
• src/univalue/lib/univalue.cpp
• src/univalue/lib/univalue_get.cpp
• src/index/coinstatsindex.cpp
• src/dbwrapper.cpp
• src/node/chainstate.cpp
• src/qt/optionsdialog.cpp
• src/dbwrapper.h
• src/index/blockfilterindex.cpp
• src/wallet/rpc/util.cpp
• src/bench/checkblock.cpp
• src/txdb.h
• src/rpc/mempool.cpp
• src/net_processing.cpp
• src/univalue/include/univalue.h
• src/wallet/rpc/backup.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/evo/evodb.cpp

ci/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

• ci/dash/lint-tidy.sh

doc/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to the doc directory (documentation)

Files:

• doc/design/libraries.md
• doc/README.md

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
• src/univalue/lib/univalue_get.cpp
• src/univalue/include/univalue.h

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

📄 CodeRabbit inference engine (CLAUDE.md)

Performance benchmarks should be placed in src/bench/ and use nanobench

Files:

• src/bench/checkblock.cpp

🧠 Learnings (1)

📚 Learning: 2025-08-08T07:01:47.332Z

Learnt from: knst
PR: dashpay/dash#6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.

Applied to files:

• test/functional/wallet_basic.py
• test/functional/wallet_multiwallet.py
• src/wallet/rpc/util.cpp
• test/functional/wallet_hd.py

🔇 Additional comments (42)

src/wallet/rpc/backup.cpp (1)

208-216: LGTM! Documentation improvement is clear and consistent.

The updated help text now specifies the exact descriptor syntax ("addr(X)") for users migrating from importaddress to importdescriptors in descriptor wallets. This brings the documentation in line with similar RPC commands like importprivkey and importpubkey, which already specify their respective descriptor syntax ("combo(X)").

test/functional/rpc_help.py (1)

95-95: LGTM! Test expectation aligns with more explicit JSON type error messages.

The updated error message "JSON value of type number is not of expected type string" is more descriptive than the previous "JSON value is not a string as expected", clearly indicating both the actual and expected types.

src/wallet/rpc/util.cpp (2)

103-103: LGTM! More specific error message for legacy wallet requirement.

The updated error message "Only legacy wallets are supported by this command" is clearer and more specific than "This type of wallet does not support this command", making it immediately obvious what wallet type is required.

---

112-112: LGTM! Consistent error message for legacy wallet requirement.

The error message update matches the change in EnsureLegacyScriptPubKeyMan for consistency across both functions.

test/functional/wallet_multiwallet.py (1)

356-356: LGTM! Test expectation updated for type-explicit error messages.

The updated error message "JSON value of type null is not of expected type string" accurately reflects that no argument (null) is being passed when a string is expected.

test/functional/mining_prioritisetransaction.py (1)

123-123: LGTM! Test expectation updated for type-explicit error messages.

The updated error message "JSON value of type string is not of expected type number" is more descriptive and aligns with JSON's type system (using "number" rather than "integer").

test/functional/wallet_descriptor.py (1)

75-83: LGTM! Test expectations updated to match improved error messages.

All test expectations have been updated to use the more specific error message "Only legacy wallets are supported by this command", which corresponds to the source code changes in src/wallet/rpc/util.cpp. The new message is clearer and more user-friendly.

test/functional/rpc_rawtransaction.py (5)

131-132: LGTM! Test expectations updated for type-explicit error messages.

Updated error message to "not of expected type bool" which is clearer than "not a boolean".

---

135-138: LGTM! Consistent type-explicit error message for boolean validation.

Both test cases now use "not of expected type bool" for invalid verbose parameter values.

---

160-160: LGTM! Type-explicit error message for blockhash parameter.

The error message "JSON value of type bool is not of expected type string" clearly indicates both the actual and expected types.

---

190-192: LGTM! Type-explicit error messages for createrawtransaction parameters.

Updated error messages for invalid inputs parameters:

• Line 190: "JSON value of type string is not of expected type object" for invalid input object
• Line 191: "JSON value of type null is not of expected type string" for missing txid

---

216-216: LGTM! Type-explicit error message for outputs parameter.

The error message "JSON value of type string is not of expected type array" clearly indicates the type mismatch.

test/functional/wallet_basic.py (1)

456-456: LGTM! Test expectation updated for type-explicit error message.

The updated error message "not of expected type number" is more descriptive and aligns with JSON's type system.

test/functional/test_framework/test_framework.py (1)

684-688: LGTM! Simplified test framework API.

The removal of the expected_stderr parameter from stop_nodes simplifies the test framework. Individual tests can still verify stderr by calling stop_node directly on specific nodes (as shown in line 680).

Verify that no test files pass expected_stderr to stop_nodes:

test/functional/wallet_hd.py (1)

187-188: Updated RPC type-error expectations look correct

Strings match new checkType() wording (“string”→“bool” and “bool”→“string”).

src/univalue/lib/univalue_get.cpp (1)

49-51: Centralized type checks via checkType(): LGTM

Consistent, concise, and aligns errors with the new message format.

Also applies to: 62-64, 68-70, 74-79, 83-85, 89-91

test/functional/rpc_blockchain.py (1)

266-266: Updated type-error expectations: LGTM

Messages correctly reflect “string↔number” mismatches per new type-checking.

Also applies to: 271-271, 546-546

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

106-109: Centralized VType checks in mutators: LGTM

Cleaner validation and uniform error strings across object/array operations.

Also applies to: 113-116, 120-124, 128-135, 139-144

---

210-216: checkType() implementation: LGTM

Error text matches tests (“JSON value of type <uvTypeName(typ)> is not of expected type <uvTypeName(expected)>”).

doc/README.md (1)

61-61: LGTM!

The new link to Internal Design Docs provides a clear entry point to the design documentation directory, which aligns well with the new doc/design/libraries.md file being added in this PR.

configure.ac (1)

2002-2004: LGTM!

The AC_CONFIG_LINKS additions follow standard autoconf patterns for out-of-tree builds, creating necessary symlinks from the build directory to the source directory for Qt and test Makefiles. This is consistent with the existing AC_CONFIG_LINKS entries at lines 1995-2001.

doc/design/libraries.md (1)

3-16: Clarify library naming consistency.

At line 8, *libbitcoinconsensus* is listed as the shared library build of *libdash_consensus* (line 7). Should the shared library be named *libdashconsensus* instead to maintain consistency with the Dash branding, or is the *libbitcoinconsensus* naming intentional for backward compatibility?

src/chain.h (1)

124-124: Verify that the referenced documentation file exists.

The path has been updated from doc/assumeutxo.md to doc/design/assumeutxo.md, but the target file is not visible in this PR. Ensure that doc/design/assumeutxo.md exists or is being created as part of the design documentation reorganization.

src/net_processing.cpp (6)

419-420: Switch to NodeClock::time_point for getheaders timestamp looks correct

Guarded by g_msgproc_mutex, default-init sentinel preserves prior semantics. Reads/writes are confined under the mutex.

---

614-616: NewPoWValidBlock lock contract updated to avoid inversion

Requiring not holding m_most_recent_block_mutex on entry matches the internal locking order (cs_main then m_most_recent_block_mutex).

---

621-624: Lock annotations order updated for message processing

Including g_msgproc_mutex and ordering it after !m_most_recent_block_mutex aligns with the intended lock hierarchy for the message thread.

Also applies to: 643-644

---

1038-1040: ProcessGetData lock requirements clarified

Requiring peer.m_getdata_requests_mutex (and not holding m_most_recent_block_mutex / cs_main) matches call sites and avoids inversions.

---

3022-3031: Use NodeClock::now and duration compare for getheaders throttle

Clock migration is consistent; comparison to HEADERS_RESPONSE_TIME is correct. Timestamp updated only under g_msgproc_mutex.

---

5000-5001: Resetting last getheaders timestamp on HEADERS receipt is correct

Default-constructing the time_point (no outstanding request) preserves the “allow immediate follow-up” behavior.

src/index/blockfilterindex.cpp (1)

12-12: LGTM! Include makes dependency explicit.

The addition of <util/system.h> is necessary since this file uses gArgs.GetDataDirNet() at Line 111. This change aligns with the PR's IWYU (include-what-you-use) refactoring effort to make dependencies explicit rather than relying on transitive includes.

src/evo/specialtxman.cpp (1)

14-14: LGTM! Header addition aligns with IWYU refactoring.

The addition of <util/system.h> makes dependencies explicit as part of the broader include-what-you-use refactoring across the codebase.

src/index/coinstatsindex.cpp (1)

13-13: LGTM! Include is necessary for gArgs usage.

The addition of <util/system.h> is required since this file uses gArgs.GetDataDirNet() at Line 108. This change is part of the IWYU refactoring to make dependencies explicit.

src/index/base.cpp (1)

11-11: LGTM! Header addition improves dependency clarity.

The addition of <util/system.h> makes dependencies explicit as part of the IWYU refactoring effort across the codebase.

src/qt/optionsdialog.cpp (2)

26-26: LGTM! Header addition follows IWYU pattern.

The addition of <util/system.h> makes dependencies explicit, consistent with the broader IWYU refactoring across the codebase.

---

31-31: LGTM! Header reordering improves organization.

Moving the <QButtonGroup> include improves the organization of Qt-related includes.

src/evo/evodb.cpp (1)

6-6: LGTM! Include is necessary for gArgs usage.

The addition of <util/system.h> is required since this file uses gArgs.GetDataDirNet() at Line 36. This change makes the dependency explicit as part of the IWYU refactoring.

src/bench/checkblock.cpp (1)

12-12: LGTM! Include is necessary for ArgsManager usage.

The addition of <util/system.h> is required since this file uses ArgsManager at Line 41. This change makes the dependency explicit as part of the IWYU refactoring.

src/rpc/mempool.cpp (1)

19-20: LGTM! Both includes are necessary.

The additions of <util/system.h> and <util/time.h> are required:

• util/system.h is needed for gArgs.GetIntArg() usage at Line 609
• util/time.h is needed for count_seconds() usage at Line 284

These changes make dependencies explicit as part of the IWYU refactoring effort.

src/txdb.h (1)

12-12: Include adds required lock annotations/types — LGTM

Brings in RecursiveMutex and thread-safety annotations used below (ResizeCache, LoadBlockIndexGuts). Safe and correct.

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

24-26: Extending IWYU coverage — LGTM

Including dbwrapper.cpp and node/chainstate.cpp keeps include hygiene aligned with the broader changes.

src/node/chainstate.cpp (2)

7-18: Include additions for locks/time/hash — LGTM

Adds sync/threadsafety/time/uint256 needed for annotations and types used below. Matches usage.

Also applies to: 27-32

---

33-64: Ensure mempool is non-null at call sites

mempool is dereferenced in DashChainstateSetup (ConnectManagers). If blocks-only or other modes can pass nullptr, this will crash. Please confirm all callers pass a valid mempool or guard accordingly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Missing required standard headers used in this header (compile/IWYU break risk)

This header publicly uses std::map, std::set, std::unique_ptr/make_unique, std::remove_pointer, and std::lexicographical_compare but does not include , , , <type_traits>, or . Strict builds or IWYU will fail/complain. Add the missing headers.

Additionally, ssize_t is used later; ensure it’s defined on all platforms (either include the appropriate header or switch to std::ptrdiff_t). The latter is recommended but optional.

Apply:

 #include <typeindex>
+#include <algorithm>
+#include <map>
+#include <memory>
+#include <set>
+#include <type_traits>
 
 #include <cstddef>
 #include <cstdint>
 #include <exception>

Optional (for portability; alternatively include the platform header that defines ssize_t):

• Replace ssize_t memoryUsage{0}; with std::ptrdiff_t memoryUsage{0};

📝 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

	#include <cstddef>
	#include <cstdint>
	#include <exception>
	#include <leveldb/db.h>
	#include <leveldb/iterator.h>
	#include <leveldb/options.h>
	#include <leveldb/slice.h>
	#include <leveldb/status.h>
	#include <leveldb/write_batch.h>
	#include <stdexcept>
	#include <string>
	#include <vector>
	namespace leveldb {
	class Env;
	}
	#include <algorithm>
	#include <map>
	#include <memory>
	#include <set>
	#include <type_traits>
	#include <cstddef>
	#include <cstdint>
	#include <exception>
	#include <leveldb/db.h>
	#include <leveldb/iterator.h>
	#include <leveldb/options.h>
	#include <leveldb/slice.h>
	#include <leveldb/status.h>
	#include <leveldb/write_batch.h>
	#include <stdexcept>
	#include <string>
	#include <vector>
	namespace leveldb {
	class Env;
	}

🤖 Prompt for AI Agents

In src/dbwrapper.h around lines 17 to 32, this header publicly uses std::map,
std::set, std::unique_ptr/make_unique, std::remove_pointer, and
std::lexicographical_compare but doesn't include the required standard headers;
add #include <map>, #include <set>, #include <memory>, #include <type_traits>,
and #include <algorithm> near the other includes. Also replace any use of
ssize_t (e.g., memoryUsage) with std::ptrdiff_t to avoid platform-specific
ssize_t dependency (or alternatively include the appropriate platform header),
and update forward declarations/usages accordingly so the header compiles under
strict/IWYU builds.

[knst]

converted to draft, expected to fail on iwyu linter for dbwrapper with error:

dbwrapper.h should add these lines:
#include <assert.h>               // for assert
#include <sys/types.h>            // for ssize_t
#include <algorithm>              // for lexicographical_compare
#include <map>                    // for map
#include <memory>                 // for unique_ptr, make_unique
#include <set>                    // for set
#include <type_traits>            // for remove_pointer
#include <utility>                // for move

dbwrapper.h should remove these lines:
- #include <typeindex>  // lines 15-15

https://github.com/dashpay/dash/actions/runs/18554460629/job/52888698313?pr=6897

But it success. Going to split PR to all other backports and PR with iwyu fixes

[UdjinM6]

utACK fc3e8cc