backport: merge bitcoin#24098, #26143, #24576, #24739, #25011, #25045, #25106, #25213, #25003, #24629, #24640, #24913, #24924, #25023, #25887, #25810, #25792, #25227, #27218 (auxiliary backports: part 26) #6784
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
kwvg
changed the title
kwvg
force-pushed
the
auxports26
branch
3 times, most recently
from
7452e36 to
43d36ff
Compare
kwvg
changed the title
kwvg
force-pushed
the
auxports26
branch
2 times, most recently
from
37a9f92 to
9665507
Compare
kwvg
changed the title
…n `interface_rest`
…et_createwallet
…erheaders` (REST)
WalkthroughThis set of changes includes code, test, and documentation updates across multiple areas. REST API endpoints for block headers and blockfilter headers now accept a Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–50 minutes This review involves numerous medium-complexity changes across core, test, and documentation files. The scope includes new features (REST query parameters, hex parsing), interface changes (REST format parsing, Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/util/strencodings.cpp (1)
99-100: Fix template instantiation references.The template instantiations still reference
ParseHexbut should referenceTryParseHexsince the function was renamed.-template std::vector<std::byte> ParseHex(std::string_view); -template std::vector<uint8_t> ParseHex(std::string_view); +template std::optional<std::vector<std::byte>> TryParseHex(std::string_view); +template std::optional<std::vector<uint8_t>> TryParseHex(std::string_view);
🧹 Nitpick comments (4)
src/validation.cpp (1)
2974-2977: Dereferencing pointer for the newSetTip()signature – double-check callers
CChain::SetTip()now expects a reference, so the call sites were switched to:m_chain.SetTip(*somePointer);This is fine for the four modified spots (
DisconnectTip,ConnectTip,LoadChainTip,PopulateAndValidateSnapshot) because each pointer is proven non-null just above the call (either viaassert, early-return, or function pre-conditions).Suggestion (optional): for consistency add
assert(pindex)before the call inLoadChainTip()andPopulateAndValidateSnapshot()the same way it’s done inDisconnectTip().Also applies to: 3105-3108, 4450-4453, 5829-5832
test/functional/test_framework/messages.py (1)
68-73: Centralizing mempool/OP_RETURN defaults looks correct, just keep them in sync with policy codeThe chosen values (25/25 ancestor–descendant limits and 83-byte OP_RETURN relay size) match Dash / Bitcoin Core defaults, so the change removes hard-coded literals across the test suite — good move.
If the policy defaults ever change upstream, remember these constants will silently diverge; consider adding a single source-of-truth import from the C++ policy header in the future to avoid drift.test/functional/wallet_createwallet.py (1)
32-32: LGTM! Test simplification focuses on core behavior.Removing the
descriptors=Trueparameter simplifies the test to focus specifically on the interaction between passphrases and disabled private keys without introducing unrelated descriptor functionality.However, there's an indentation issue flagged by static analysis:
- self.nodes[0].createwallet, wallet_name='w0', disable_private_keys=True, passphrase="passphrase") + self.nodes[0].createwallet, wallet_name='w0', disable_private_keys=True, passphrase="passphrase")test/functional/mempool_packages.py (1)
261-261: Address the indentation issue flagged by static analysis.The line continuation is over-indented. Consider aligning with the opening parenthesis:
self.wait_until(lambda: len(self.nodes[1].getrawmempool()) == - CUSTOM_ANCESTOR_LIMIT + 1 + CUSTOM_DESCENDANT_LIMIT, timeout=10) + CUSTOM_ANCESTOR_LIMIT + 1 + CUSTOM_DESCENDANT_LIMIT, timeout=10)
…NCESTOR,DESCENDANT}_LIMIT
The next commit will no longer treat invalid encodings as representing valid data, hex messages cannot be odd length.
kwvg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/mempool_packages.py (1)
261-261: Fix indentation issue.The static analysis tool correctly identified an over-indentation issue on this line.
- CUSTOM_ANCESTOR_LIMIT + 1 + CUSTOM_DESCENDANT_LIMIT, timeout=10) + CUSTOM_ANCESTOR_LIMIT + 1 + CUSTOM_DESCENDANT_LIMIT, timeout=10)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
doc/release-notes-24629.md(1 hunks)src/Makefile.bench.include(1 hunks)src/bench/wallet_loading.cpp(1 hunks)src/chain.cpp(1 hunks)src/chain.h(1 hunks)src/init.cpp(2 hunks)src/rpc/blockchain.cpp(3 hunks)src/test/evo_assetlocks_tests.cpp(1 hunks)src/test/miner_tests.cpp(3 hunks)src/test/skiplist_tests.cpp(3 hunks)src/test/util_tests.cpp(1 hunks)src/util/strencodings.cpp(1 hunks)src/util/strencodings.h(1 hunks)src/validation.cpp(5 hunks)src/wallet/walletdb.cpp(1 hunks)src/wallet/walletdb.h(1 hunks)test/functional/feature_index_prune.py(3 hunks)test/functional/feature_pruning.py(1 hunks)test/functional/mempool_datacarrier.py(1 hunks)test/functional/mempool_package_onemore.py(3 hunks)test/functional/mempool_packages.py(9 hunks)test/functional/test_framework/messages.py(1 hunks)test/functional/test_runner.py(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- src/Makefile.bench.include
- doc/release-notes-24629.md
- src/test/skiplist_tests.cpp
- test/functional/test_runner.py
- src/test/miner_tests.cpp
- test/functional/feature_index_prune.py
- test/functional/test_framework/messages.py
- test/functional/mempool_package_onemore.py
🚧 Files skipped from review as they are similar to previous changes (13)
- src/wallet/walletdb.h
- src/chain.cpp
- src/chain.h
- src/wallet/walletdb.cpp
- src/init.cpp
- src/util/strencodings.h
- src/test/evo_assetlocks_tests.cpp
- test/functional/mempool_datacarrier.py
- test/functional/feature_pruning.py
- src/validation.cpp
- src/util/strencodings.cpp
- src/rpc/blockchain.cpp
- src/bench/wallet_loading.cpp
🧰 Additional context used
📓 Path-based instructions (3)
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/{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
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/mempool_packages.py
🧠 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: UdjinM6
PR: dashpay/dash#6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests for c++ code should b...
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : 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
Applied to files:
src/test/util_tests.cpp
📚 Learning: in the test framework consolidation pr (#6718), user kwvg prefers to limit functional changes to tho...
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/mempool_packages.py
📚 Learning: the updatetransactionsfromblock() method in txmempool.cpp takes parameters in the order: vhashupdate...
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Applied to files:
test/functional/mempool_packages.py
📚 Learning: pull request #6543 is focused on move-only changes and refactoring, specifically backporting from bi...
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.
Applied to files:
test/functional/mempool_packages.py
🧬 Code Graph Analysis (1)
test/functional/mempool_packages.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)src/node/interfaces.cpp (12)
tx(512-515)tx(512-512)tx(635-638)tx(635-635)tx(639-642)tx(639-639)tx(660-663)tx(660-660)tx(905-912)tx(905-905)tx(924-939)tx(924-924)
🪛 Flake8 (7.2.0)
test/functional/mempool_packages.py
[error] 261-261: continuation line over-indented for visual indent
(E127)
⏰ 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_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (15)
test/functional/mempool_packages.py (6)
10-14: LGTM! Clean import of centralized constants.The addition of
DEFAULT_ANCESTOR_LIMITandDEFAULT_DESCENDANT_LIMITimports aligns with centralizing mempool constants across the test framework, improving maintainability.
25-27: Good renaming for consistency.Renaming the custom constants to
CUSTOM_ANCESTOR_LIMITandCUSTOM_DESCENDANT_LIMITprovides better clarity and consistency with the imported default constants.
40-41: Correct usage of renamed constants.The node configuration properly uses the renamed custom constants for setting the mempool limits.
61-92: All constant references updated correctly.The test logic properly uses
DEFAULT_ANCESTOR_LIMITthroughout this section for generating transaction chains and validating mempool state. The logic remains functionally identical while using the centralized constants.
209-211: Consistent usage of custom constants.The test correctly uses
CUSTOM_ANCESTOR_LIMITwhen validating node1's mempool behavior with its custom ancestor limit configuration.
236-268: Descendant limit testing updated correctly.All references to descendant limits have been properly updated to use
DEFAULT_DESCENDANT_LIMITandCUSTOM_DESCENDANT_LIMITas appropriate for the test scenarios.src/test/util_tests.cpp (9)
148-149: LGTM! Consistent test pattern for TryParseHex basic functionality.The test correctly validates that
TryParseHexproduces the same result asParseHexfor valid hex input, using the appropriate pattern with.value()extraction.
154-155: LGTM! Proper validation of space handling in TryParseHex.The test correctly verifies that spaces between bytes are supported, maintaining consistency with the existing ParseHex behavior.
160-161: LGTM! Appropriate test for leading space support.The test correctly validates leading space handling, which is important for BerkeleyEnvironment::Salvage compatibility as noted in the comment.
163-167: LGTM! Excellent addition of mixed case and spacing test coverage.The new test case properly validates case-insensitive hex parsing with irregular spacing, enhancing the test suite's coverage of edge cases.
169-173: LGTM! Good coverage for empty string edge case.The test correctly validates that both parsing functions handle empty input gracefully by returning empty results.
175-177: LGTM! Correct validation of different error handling approaches.The test properly validates that spaces between nibbles are treated as invalid, with ParseHex returning empty vector and TryParseHex returning nullopt.
184-185: LGTM! Proper validation of embedded null rejection.The test correctly ensures that embedded null characters are treated as invalid input, with appropriate error handling for both functions.
187-189: LGTM! Appropriate validation of non-hex character rejection.The test correctly ensures that strings containing non-hex characters are properly rejected by both parsing functions.
191-193: LGTM! Important validation for truncated hex input.The test correctly ensures that incomplete hex bytes (odd number of hex digits) are properly rejected, which is crucial for data integrity.
4 participants
Additional information
The documentation introduced in bitcoin#24098 has been updated to reflect the affected version as v23.
In bitcoin#24576, while the definitions
OP_{0,1,2,16}are removed, they were not subsequently imported fromtest_framework.scriptas they are unused ingen_key_io_test_vectors.pyIn bitcoin#25106,
utf8string()is called instead ofu8string()due to bitcoin#29040 (backported as 209488d in dash#6657) updating the function name.Due to stricter validation introduced in bitcoin#25227, the unit test
evo_assetlockfails (see below) due toParseHexrefusing to treat malformed hex encoding as representing valid data and the unit test used a dummy message of improper length.Test failure:
This has been resolved in a separate commit.
Breaking Changes
pruneblockchainmethod had an off-by-one bug, returning the height of the block after the most recent pruned. This has been corrected, and it now returns the height of the last pruned block as documented.Checklist