backport: batch jul 24 2025 #6780
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
PastaPastaPasta
force-pushed
the
backport-batch-jul-24-2025
branch
from
87d60f4 to
3596986
Compare
WalkthroughThis set of changes includes documentation updates, test improvements, and code modifications across the wallet, RPC, utility, and GUI components. Documentation files were updated for clarity, formatting, and to reflect new or changed RPC fields and behaviors, such as the addition of the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Complexity label: Moderate Rationale: 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: 0
🧹 Nitpick comments (2)
doc/release-process.md (1)
70-76: Clarify tag-fetching to guarantee the tag actually exists locally
git fetch origin "v${VERSION}"only downloads the object toFETCH_HEAD; it does not create a local tag, so the subsequentgit checkout "v${VERSION}"will fail for first-time builders.
Suggest fetching the tag explicitly (or all tags) so it materialises locally:-git fetch origin "v${VERSION}" +# Either grab the single tag … +git fetch --depth=1 origin "refs/tags/v${VERSION}:refs/tags/v${VERSION}" +# … or, if bandwidth isn’t a concern, fetch them all: +# git fetch --tags originThis guarantees
git checkout "v${VERSION}"works out-of-the-box and avoids confusion for new builders.doc/release-notes-27068.md (1)
1-6: LGTM! Clear documentation of important wallet behavior change.The release note accurately documents the change to wallet passphrase handling, clearly explaining the new support for null characters and the previous limitation.
Optional style improvement per static analysis:
- Prior to this change, only characters up to the first + Before this change, only characters up to the first
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9f3e4fa and 2b7ba5f169cd3d973c6a28eed5beea33f8665a34.
📒 Files selected for processing (24)
ci/test/00_setup_env_native_valgrind.sh(1 hunks)ci/test/00_setup_env_s390x.sh(1 hunks)contrib/valgrind.supp(1 hunks)doc/descriptors.md(1 hunks)doc/release-notes-25158.md(1 hunks)doc/release-notes-27068.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.qt.include(1 hunks)src/crypto/sha256_arm_shani.cpp(1 hunks)src/hash.h(1 hunks)src/init.cpp(1 hunks)src/node/blockstorage.cpp(3 hunks)src/qt/askpassphrasedialog.cpp(3 hunks)src/support/allocators/secure.h(1 hunks)src/support/lockedpool.cpp(1 hunks)src/support/lockedpool.h(1 hunks)src/wallet/rpc/encrypt.cpp(4 hunks)src/wallet/rpc/transactions.cpp(4 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)test/functional/feature_block.py(23 hunks)test/functional/p2p_disconnect_ban.py(2 hunks)test/functional/wallet_abandonconflict.py(1 hunks)test/functional/wallet_encryption.py(1 hunks)
🧰 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/support/lockedpool.cppsrc/wallet/test/wallet_tests.cppsrc/support/allocators/secure.hsrc/support/lockedpool.hsrc/crypto/sha256_arm_shani.cppsrc/node/blockstorage.cppsrc/wallet/rpc/transactions.cppsrc/hash.hsrc/wallet/rpc/encrypt.cppsrc/wallet/rpc/wallet.cppsrc/init.cppsrc/qt/askpassphrasedialog.cpp
doc/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-25158.mddoc/release-notes-27068.mddoc/descriptors.mddoc/release-process.md
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_s390x.shci/test/00_setup_env_native_valgrind.sh
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/wallet/test/wallet_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/wallet_abandonconflict.pytest/functional/feature_block.pytest/functional/wallet_encryption.pytest/functional/p2p_disconnect_ban.py
contrib/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/valgrind.supp
🧠 Learnings (17)
📓 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: 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#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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
doc/release-notes-25158.md (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #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.
ci/test/00_setup_env_s390x.sh (1)
Learnt from: kwvg
PR: #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.
src/wallet/test/wallet_tests.cpp (4)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #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: 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
Learnt from: kwvg
PR: #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.
doc/release-notes-27068.md (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/functional/wallet_abandonconflict.py (2)
Learnt from: kwvg
PR: #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: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/functional/feature_block.py (2)
Learnt from: kwvg
PR: #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: #6726
File: test/functional/rpc_createmultisig.py:120-120
Timestamp: 2025-06-20T23:32:16.225Z
Learning: In the rpc_createmultisig.py test, the checkbalances() function correctly excludes 9 blocks from the balance calculation: 8 blocks from do_multisig() calls (2 blocks per call × 4 calls) plus 1 block from checkbalances() itself.
ci/test/00_setup_env_native_valgrind.sh (2)
Learnt from: kwvg
PR: #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: #6516
File: ci/test/00_setup_env_mac.sh:11-11
Timestamp: 2025-01-02T08:33:26.751Z
Learning: The removal of DMG support in the macOS packaging process eliminates the need for python-based scripts or python3-setuptools in the build environment. The PACKAGES variable in ci/test/00_setup_env_mac.sh is vestigial due to the use of a general-purpose Docker container defined in contrib/containers/ci/Dockerfile.
test/functional/wallet_encryption.py (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/node/blockstorage.cpp (1)
Learnt from: kwvg
PR: #6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/is_basic_scheme_active=/true, /is_extended_addr=/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
src/wallet/rpc/transactions.cpp (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
src/hash.h (1)
Learnt from: kwvg
PR: #6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
src/Makefile.qt.include (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to 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
src/wallet/rpc/encrypt.cpp (2)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: #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.
doc/release-process.md (2)
Learnt from: UdjinM6
PR: dashpay/dash#0
File: :0-0
Timestamp: 2025-02-13T07:36:17.572Z
Learning: In GitHub Actions checkout action, fetch-depth: 0 fetches all commit history for all branches and tags, while a positive number like fetch-depth: 50 fetches only that many recent commits, which is useful for CI optimization.
Learnt from: kwvg
PR: #6754
File: contrib/containers/guix/docker-compose.yml:18-19
Timestamp: 2025-07-14T10:11:05.011Z
Learning: In the Guix build process for Dash Core, the guix.sigs directory requires write access as signatures are written to it during the build process, and dash-detached-sigs may be updated with git pull operations, so both directories need rw permissions in the Docker volume mounts.
src/wallet/rpc/wallet.cpp (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/qt/askpassphrasedialog.cpp (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
🧬 Code Graph Analysis (1)
test/functional/wallet_encryption.py (2)
src/wallet/rpc/encrypt.cpp (6)
walletlock(179-214)walletlock(179-179)walletpassphrasechange(126-177)walletpassphrasechange(126-126)walletpassphrase(10-124)walletpassphrase(10-10)test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)
🪛 LanguageTool
doc/release-notes-27068.md
[style] ~5-~5: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...ases may now contain null characters. Prior to this change, only characters up to the ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
⏰ 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: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (29)
src/Makefile.qt.include (3)
440-441: Good documentation practice.The added comments clearly explain the purpose of the translation workflow changes and provide a helpful reference link to Transifex documentation for distinguishing between source and translation files.
446-446: Appropriate flag for source file generation.The
-drop-translationsflag correctly ensures that existing translations are removed from the input.tsfile, which is necessary for generating a proper source.xlffile rather than a translation file.
447-448: Effective post-processing for Transifex compliance.The sed commands properly transform the generated
.xlffile to meet Transifex source file requirements by:
- Removing the redundant
target-language="en"attribute- Eliminating empty
<target>tags- Cleaning up the backup file created during the process
The approach using sed with in-place editing and backup cleanup is appropriate for this build system context.
src/support/lockedpool.h (1)
8-12: Removal of<stdint.h>may break transitive dependencies – please double-check downstream headersThe header itself only uses
size_t, so replacing<stdint.h>with<cstddef>is correct here.
However, other compilation units that#include "support/lockedpool.h"might have been relying on the transitive inclusion of<stdint.h>for fixed-width integer types (uint8_t,int32_t, …). If any of them omitted their own<cstdint>/<stdint.h>include, this change will surface as a missing-type compile error.No action is required in this file, but please run a full build or at least grep for translation units that:
#include "support/lockedpool.h"- use
int*_t/uint*_ttypes- do not include
<cstdint>/<stdint.h>themselves.If such files exist, add the proper include there to keep dependencies explicit.
src/support/lockedpool.cpp (1)
22-22: Good catch adding<limits>– resolves implicitstd::numeric_limitsuseThe new include fixes the missing declaration problem cleanly; no further issues spotted.
doc/descriptors.md (1)
76-77: Formatting change looks goodTurning the hardened-suffix remark into a parenthetical detaches it from the bullet list without altering meaning. No further action needed.
src/init.cpp (1)
748-749: LGTM! Improved argument categorization.Moving
-permitbaremultisigfrom CONNECTION to NODE_RELAY category makes logical sense as this argument controls transaction relay policy rather than general connection settings. The change improves command-line argument organization without affecting functionality.test/functional/feature_block.py (1)
174-174: LGTM! Clean code improvement by removing unused variables.The removal of unused variable assignments from
self.next_block()calls improves code readability and eliminates dead code. The test logic remains intact since these variables were either unused or reassigned later in the code flow.Also applies to: 685-685, 765-765, 782-782, 816-816, 828-828, 864-864, 889-889, 970-970, 983-983, 997-997, 1018-1018, 1038-1038, 1056-1056, 1094-1094, 1122-1122, 1135-1135, 1150-1150, 1178-1178, 1184-1184, 1189-1189, 1221-1221, 1240-1240, 1278-1278
src/hash.h (1)
171-202: LGTM! Well-designed template class for hash verification.The new
HashVerifiertemplate class provides a clean, modern alternative to the existingCHashVerifier. Key improvements include:
- Uses a reference instead of pointer for safer source handling
- LIFETIMEBOUND attribute prevents lifetime issues
- Inherits from the simpler
HashWriterbase class- Template design allows flexibility with different source types
- Clean interface with
read,ignore, and extraction operator methodsThe implementation is correct and follows good C++ practices.
src/crypto/sha256_arm_shani.cpp (1)
65-65: LGTM! Typo fix improves documentation quality.Good catch fixing the spelling error from "implemenation" to "implementation" in the comment.
src/support/allocators/secure.h (1)
58-58: LGTM! Good documentation of potential security enhancement.The TODO comment suggests a reasonable future improvement to extend secure memory locking to RPC parameters, which would complement the existing secure string allocator functionality.
doc/release-notes-25158.md (1)
1-6: LGTM! Clear and accurate release notes.The documentation clearly describes the RPC wallet enhancement, specifying which RPCs are affected and the nature of the change. The format is consistent with standard release notes.
src/wallet/test/wallet_tests.cpp (1)
461-461: LGTM! Good catch on the typo fix.The duplicate word "the" has been correctly removed from the comment, improving readability.
test/functional/p2p_disconnect_ban.py (2)
6-6: LGTM! Appropriate import for file operations.The
pathlib.Pathimport is correctly added to support the new banlist database recreation test.
40-48: LGTM! Excellent test for banlist database resilience.The test properly verifies that the node can recreate the banlist database when the file is missing:
- Correctly stops the node and deletes the banlist.json file
- Uses
assert_debug_logto verify the expected recreation message- Confirms the file exists after restart and the banlist is empty
This test enhances the robustness verification of the banlist persistence mechanism.
ci/test/00_setup_env_s390x.sh (1)
22-22: LGTM! Consistent test exclusion for platform stability.Adding
feature_initto the excluded tests is appropriate for the s390x CI environment, maintaining consistency with other problematic test exclusions on this platform.test/functional/wallet_abandonconflict.py (1)
132-138: LGTM! Comprehensive test coverage for abandoned field.The new test section properly verifies that the "abandoned" field is present and correctly set for all transaction categories in the
listtransactionsoutput. This complements the existinglistsinceblocktest and ensures full coverage of the RPC interface changes documented in the release notes.The test logic is sound:
- Filters transactions by the abandoned txid
- Asserts transactions exist to avoid false positives
- Iterates through all categories to verify field presence and correctness
contrib/valgrind.supp (1)
16-17: LGTM: Documentation update reflects current testing environment.The update to Ubuntu 22.04 with clang accurately reflects the modernized CI infrastructure and helps maintainers understand the testing context for these suppressions.
ci/test/00_setup_env_native_valgrind.sh (1)
12-12: LGTM: Appropriate test exclusion for Valgrind environment.Adding
feature_initto the exclusion list follows the established pattern for handling tests that are problematic in Valgrind environments, helping maintain CI stability.test/functional/wallet_encryption.py (1)
90-100: LGTM: Comprehensive test for null character support in passphrases.The test properly validates the new null character functionality by:
- Ensuring truncated passphrases (before null) correctly fail
- Confirming full passphrases with embedded nulls work correctly
- Performing end-to-end validation with signing operations
This aligns well with the RPC changes for null character handling.
src/wallet/rpc/wallet.cpp (1)
599-599: LGTM: Improved passphrase handling supports null characters.The change from
.c_str()tostd::string_view{request.params[3].get_str()}correctly preserves embedded null characters in passphrases, aligning with the broader null character support improvements in wallet encryption.src/wallet/rpc/transactions.cpp (2)
412-412: LGTM: Consistent abandoned field reporting across transaction categories.The addition of the
abandonedfield to received transactions aligns with the documented behavior change, ensuring consistent reporting across all transaction types in wallet RPC responses.
479-479: LGTM: Documentation properly updated for consistent abandoned field.The help text updates correctly reflect that the
abandonedfield is now always present across all transaction categories, removing the previous restriction to only the "send" category.Also applies to: 591-591, 731-731
src/wallet/rpc/encrypt.cpp (2)
54-54: LGTM: Improved passphrase assignment using string_view.The change from
.c_str()tostd::string_viewconstruction is more efficient and cleaner, eliminating the need for temporary C-string conversion while maintaining the same security properties.Also applies to: 151-151, 155-155, 256-256
84-94: LGTM: Enhanced error handling for null characters in passphrases.The enhanced error messages provide clear guidance for users whose passphrases contain null characters, particularly those set in older software versions. The specific reference to issue bitcoin#27067 and the actionable advice improve the user experience.
Also applies to: 162-171
src/node/blockstorage.cpp (2)
496-496: LGTM: Standardized serialization version usage.Using
CLIENT_VERSIONinstead offileout.GetVersion()provides consistent serialization behavior for block and undo data operations, improving predictability and compatibility.Also applies to: 702-702
532-532: LGTM: Simplified hash verification with new HashVerifier class.The replacement of
CHashVerifier<CAutoFile>with the streamlinedHashVerifierclass simplifies the implementation while maintaining the same functionality. The comment provides helpful context for this change.src/qt/askpassphrasedialog.cpp (2)
110-112: LGTM: Consistent passphrase assignment improvement in GUI.The change from
.c_str()tostd::string_viewmatches the improvement made in the RPC interface, providing the same efficiency benefits while maintaining security properties.
187-199: LGTM: Consistent null character error handling in GUI.The enhanced error messages for null characters in passphrases provide the same helpful guidance as the RPC interface, ensuring a consistent user experience across both GUI and RPC access methods.
Also applies to: 220-231
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
some should be partial:
26649: missing some changes in src/node/blockstorage.cpp
24259: missing changes in test/functional/feature_syscall_sandbox.py
24282 does not do what it should, it's too early
@UdjinM6 what do you suggest we do about that? mark as partial and merge, or drop from PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/README.md (5)
120-127: Use a proper Markdown heading instead of bold emphasis for “Linux”.
markdownlintis flagging MD036 because**Linux**is being used as a heading.
Switch to a real heading to satisfy linting and improve document structure.-**Linux** +#### Linux
145-152: Same heading issue for the macOS section.Apply the same fix here:
-**macOS** +#### macOS
122-132: Add recommended mount options to reduce risk and avoid surprises.It’s best practice to mount a tmpfs with
nosuid,nodev(and oftenmode=1777) so no executable set-uid files or device nodes can appear on the RAM disk.-sudo mount -t tmpfs -o size=4g tmpfs /mnt/tmp/ +sudo mount -t tmpfs -o size=4g,nosuid,nodev,mode=1777 tmpfs /mnt/tmp/This also prevents accidental permission errors when multiple users run the tests on the same host.
133-138: Clarify creation of the cache sub-directory before invoking test_runner.
test_runner.pyexpects the path passed via--cachedirto exist. The snippet assumestest_runnerwill create/mnt/tmp/cache, which it does not. Add the mkdir step to avoid a cryptic failure:sudo mkdir -p /mnt/tmp/cache -test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp +test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp
149-158: Ensure the RAM-disk directories exist on macOS as well.For parity with the Linux block, explicitly create the sub-directories:
-diskutil erasevolume HFS+ ramdisk $(hdiutil attach -nomount ram://8388608) +diskutil erasevolume HFS+ ramdisk $(hdiutil attach -nomount ram://8388608) +mkdir -p /Volumes/ramdisk/{cache,tmp}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 15cb2621401c94612d8da90b0bdbf68521837d57 and 09e0f02f39e4ccfe89fee8de9f5121367adb8ce8.
📒 Files selected for processing (7)
doc/release-note-26194.md(1 hunks)src/qt/bitcoin.cpp(1 hunks)src/util/system.cpp(5 hunks)src/util/system.h(2 hunks)src/wallet/rpc/backup.cpp(2 hunks)test/README.md(1 hunks)test/functional/wallet_listdescriptors.py(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- doc/release-note-26194.md
- src/wallet/rpc/backup.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/qt/bitcoin.cppsrc/util/system.hsrc/util/system.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_listdescriptors.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: 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#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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
src/qt/bitcoin.cpp (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/functional/wallet_listdescriptors.py (1)
Learnt from: kwvg
PR: #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.
src/util/system.cpp (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/README.md (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to test/functional/**/*.py : Functional tests should be written in Python and placed in test/functional/
🧬 Code Graph Analysis (1)
src/util/system.h (1)
src/util/system.cpp (4)
GetConfigFilePath(980-983)GetConfigFilePath(980-980)EnsureDataDir(480-499)EnsureDataDir(480-480)
🪛 LanguageTool
test/README.md
[style] ~117-~117: Consider a more concise word here.
Context: ...p` directories for the functional tests in order to speed them up. Speed-up amount varies o...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
test/README.md
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
145-145: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: mac-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: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (10)
test/functional/wallet_listdescriptors.py (2)
74-75: Excellent backward compatibility implementation.This correctly includes both the legacy
'next': 0field and the new'next_index': 0field in the expected response, ensuring proper testing of backward compatibility while validating the new functionality.
89-90: Consistent backward compatibility implementation.This correctly maintains the same pattern as the public descriptor test, ensuring both the legacy
'next'and new'next_index'fields are tested consistently across all descriptor listing scenarios.src/qt/bitcoin.cpp (1)
174-174: LGTM! Good practice to ensure directory structure exists before settings operations.The call to
gArgs.EnsureDataDir()at the start ofInitSettings()is well-placed and ensures the data directory and necessary subdirectories (likewallets/) are created before any settings file operations. This aligns with the broader refactoring to centralize directory management in theArgsManagerclass.src/util/system.h (2)
261-264: LGTM! Clean interface addition for config file path access.The new
GetConfigFilePath()method provides a clean, centralized way to access the configuration file path. The method signature is appropriate (const, returns fs::path) and the documentation clearly describes its purpose.
503-507: LGTM! Well-documented directory creation method.The
EnsureDataDir()method declaration is well-designed with clear documentation explaining its purpose. The const qualifier is appropriate since it doesn't modify the ArgsManager's state, only ensures external directory structure exists. The documentation correctly notes the creation ofwallets/subdirectories.src/util/system.cpp (5)
451-452: LGTM! Simplified comment improves readability.The comment simplification removes unnecessary implementation details while retaining the essential information about caching behavior. This makes the code more maintainable.
480-499: LGTM! Well-implemented directory creation with proper wallet subdirectory handling.The
EnsureDataDir()implementation correctly:
- Creates both base and network-specific data directories when they don't exist
- Creates the
wallets/subdirectory in both locations as documented- Includes comprehensive documentation explaining the wallet directory creation rationale
- Uses proper conditional logic to only create directories when needed
The implementation aligns perfectly with the interface documentation and addresses the wallet compatibility requirements mentioned in the comments.
546-546: LGTM! Proper placement of directory initialization.Calling
EnsureDataDir()at the start ofInitSettings()ensures the necessary directory structure exists before any settings operations. This is the correct place to guarantee the environment is properly set up.
980-983: LGTM! Clean encapsulation of config file path logic.The
GetConfigFilePath()method properly encapsulates the configuration file path construction logic, improving code modularity and making it reusable across the codebase.
993-994: LGTM! Good refactoring to use the new centralized method.Using
GetConfigFilePath()instead of inline path construction improves code maintainability and consistency. The refactoring eliminates code duplication and centralizes the config file path logic.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure backward compatibility testing for the 'next' field.
The test only validates the new next_index field but doesn't verify that the legacy next field is still present for backward compatibility. According to the AI summary, both fields should be included in the response.
Consider adding an assertion to test both fields:
assert item['next_index'] == 0
+assert item['next'] == 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.
| assert item['next_index'] == 0 | |
| assert item['next_index'] == 0 | |
| assert item['next'] == 0 |
🤖 Prompt for AI Agents
In test/functional/wallet_listdescriptors.py at line 54, the test currently
asserts only the new 'next_index' field but does not check for the legacy 'next'
field required for backward compatibility. Add an assertion to verify that the
'next' field is present and has the expected value alongside the existing
'next_index' assertion to ensure both fields are tested.
We could probably just drop them for now if this would make it easier to backport them (and earlier PRs) later. |
* Merge bitcoin#26794: test: test banlist database recreation 4bdcf57 test: test banlist database recreation (brunoerg) Pull request description: This PR adds test coverage for 'banlist database recreation'. If it wasn't able to read ban db (in `LoadBanlist`), so it should create a new (an empty, ofc) one. https://github.com/bitcoin/bitcoin/blob/d8bdee0fc889def7c5f5076da13db4fce0a3728a/src/banman.cpp#L28-L45 ACKs for top commit: MarcoFalke: lgtm ACK 4bdcf57 Tree-SHA512: d9d0cd0c4b3797189dff00d3a634878188e7cf51e78346601fc97e2bf78c495561705214062bb42ab8e491e0d111f8bfcf74dbc801768bc02cf2b45f162aad85 * Apply suggestions from code review --------- Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
…pile error fadeb6b Add missing includes to fix gcc-13 compile error (MarcoFalke) Pull request description: On current master: ``` CXX support/libbitcoin_util_a-lockedpool.o support/lockedpool.cpp: In member function ‘void Arena::free(void*)’: support/lockedpool.cpp:99:20: error: ‘runtime_error’ is not a member of ‘std’ 99 | throw std::runtime_error("Arena: invalid or double free"); | ^~~~~~~~~~~~~ support/lockedpool.cpp:22:1: note: ‘std::runtime_error’ is defined in header ‘<stdexcept>’; did you forget to ‘#include <stdexcept>’? 21 | #include <algorithm> +++ |+#include <stdexcept> 22 | #ifdef ARENA_DEBUG support/lockedpool.cpp: In member function ‘void LockedPool::free(void*)’: support/lockedpool.cpp:320:16: error: ‘runtime_error’ is not a member of ‘std’ 320 | throw std::runtime_error("LockedPool: invalid address not pointing to any arena"); | ^~~~~~~~~~~~~ support/lockedpool.cpp:320:16: note: ‘std::runtime_error’ is defined in header ‘<stdexcept>’; did you forget to ‘#include <stdexcept>’? ACKs for top commit: hebasto: ACK fadeb6b. fanquake: ACK fadeb6b - tested this fixes compilation with GCC 13. I don't think theres a need to do anything else here, and that'd also just potentially complicate backporting. Tree-SHA512: 99f79cf385c913138a9cf9fc23be0a3a067b0a28518b8bdc033a7220b85bbc5d18f5356c5bdad2f628c22abb87c18b232724f606eba6326c031518559054be31 Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Claude Code <claude@anthropic.com>
…ies of transaction in ListTransaction * Merge bitcoin#23810: docs: avoid C-style casts; use modern C++ casts * Merge bitcoin#25158: rpc, wallet: add abandoned field for all categories of transaction in ListTransaction * test: add test for abandoned field in all transaction categories in listtransactions This adds a test to verify that the 'abandoned' field is now present for all transaction categories in listtransactions RPC, not just 'send'. This ensures compatibility with bitcoin#25158 which made the abandoned field available for all categories. * fix: remove scope creep from bitcoin#25158 Bitcoin commit 91ccb62 only added the 'abandoned' field to all transaction categories and updated documentation. The changes to feature_block.py were unrelated stylistic improvements (removing unused variable assignments) that constitute scope creep. This preserves Bitcoin's intent by keeping only the core functionality change and the appropriate test coverage in wallet_abandonconflict.py (which replaces the missing wallet_bumpfee.py test since Dash doesn't have RBF functionality). Fixes size validation: reduced from 342.9% to appropriate range. --------- Co-authored-by: Claude Code <claude@anthropic.com>
…t fetch" 876b91c release-process: Specify remote name in "git fetch" (Jeremy Rand) Pull request description: Avoids "does not appear to be a git repository" error. Fixes bitcoin#24329 ACKs for top commit: shaavan: ACK 876b91c Tree-SHA512: 0ba23cd51ca761823cab19200b69f07a5c23e1a501114e0af73b24195c306cebb789e187dd436f7b3895a10de31e41276bb2fc4b217cd152247d2704e44bc8da Co-authored-by: MarcoFalke <falke.marco@gmail.com>
…oser to KEY section bac30e8 docs: Move explanation of hardened key syntax closer to KEY section (Bitcoin Hodler) Pull request description: The line about "(Anywhere a `'` suffix is permitted to denote hardened derivation, the suffix `h` can be used instead.)" belongs with the section on KEY expressions, not following the unrelated TREE section. ACKs for top commit: prusnak: ACK bac30e8 meshcollider: ACK bac30e8 Tree-SHA512: 56fe97b89c02e67e94cab33b01e56f17f9b501b97036c5b35939dc4000a9d5e9afe4333869ba97bbe81372c538b7b2021a7d2520aba731400d8d0e62714d52b4 Co-authored-by: fanquake <fanquake@gmail.com>
Co-authored-by: Taeik Lim <sibera21@gmail.com>
…urce 985d85e Follow Transifex docs to prepare XLIFF source (Hennadii Stepanov) Pull request description: This PR is a bitcoin#21694 follow up. From the Transifex [docs](https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file): > A source file is different than a translation file. The translation file contains \<Target> references, whereas a source file does not. This PR makes the `qt/locale/bitcoin_en.xlf` source file according to the docs. ACKs for top commit: laanwj: ACK 985d85e Tree-SHA512: 537ef78f39a12f094b17fc902c5b59c7ae4d27e5fa35fbf6b33386843e4521236dac3275ba26ff2d1c435e53e8942284644d5de67b0b3543cec96cbcd4351607 Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>
PastaPastaPasta
force-pushed
the
backport-batch-jul-24-2025
branch
from
09e0f02 to
733c1fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
doc/release-notes-27068.md (1)
4-6: Tighten wording & follow release-note formatting convention“Prior to this change” can be shortened to “Before this change” to avoid wordiness (per LanguageTool).
Additionally, Dash/Bitcoin release notes usually keep continuation lines indented under the initial bullet:- - Wallet passphrases may now contain null characters. - Prior to this change, only characters up to the first - null character were recognized and accepted. (#27068) + - Wallet passphrases may now contain null characters. + Before this change, only characters up to the first NUL + character were recognized and accepted. (#27068)Purely stylistic—feel free to ignore if you’re keeping exact back-ported wording.
test/README.md (3)
115-118: Drop filler phrasing for concisenessThe wording “in order to” and the hyphenated “Speed-up” are flagged by LanguageTool as unnecessary verbosity. A leaner sentence improves readability without losing meaning.
-If you have available RAM on your system you can create a RAM disk to use as the `cache` and `tmp` directories for the functional tests in order to speed them up. -Speed-up amount varies on each system (and according to your RAM speed and other variables), but a 2-3x speed-up is not uncommon. +If you have spare RAM you can create a RAM disk for the functional-test `cache` and `tmp` directories to speed them up. +Depending on hardware, a 2-3× speed-up is common.
120-121: Replace bold text with proper sub-headings to satisfy MD036
markdownlint(MD036) warns against using emphasis instead of a heading.
Using an H5 sub-heading maintains hierarchy and removes the linter noise.-**Linux** +##### Linux … -**macOS** +##### macOSAlso applies to: 145-146
160-164: Usediskutil unmount/hdiutil detachon macOS
umountis available but not the usual tool on macOS.
Apple utilities provide clearer feedback and handle device node cleanup.-umount /Volumes/ramdisk +# Either of the following is more idiomatic: +# diskutil unmount /Volumes/ramdisk +# hdiutil detach /Volumes/ramdisk
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2b7ba5f169cd3d973c6a28eed5beea33f8665a34 and 733c1fdeeeaf894ceefe02c16860fe37ea1fe1f0.
📒 Files selected for processing (28)
doc/descriptors.md(1 hunks)doc/release-note-26194.md(1 hunks)doc/release-notes-25158.md(1 hunks)doc/release-notes-27068.md(1 hunks)doc/release-process.md(1 hunks)src/Makefile.qt.include(1 hunks)src/crypto/sha256_arm_shani.cpp(1 hunks)src/init.cpp(1 hunks)src/qt/askpassphrasedialog.cpp(3 hunks)src/qt/bitcoin.cpp(1 hunks)src/support/allocators/secure.h(1 hunks)src/support/lockedpool.cpp(1 hunks)src/support/lockedpool.h(1 hunks)src/util/system.cpp(5 hunks)src/util/system.h(2 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/encrypt.cpp(4 hunks)src/wallet/rpc/transactions.cpp(4 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)test/README.md(1 hunks)test/functional/feature_block.py(23 hunks)test/functional/p2p_disconnect_ban.py(2 hunks)test/functional/rpc_psbt.py(1 hunks)test/functional/wallet_abandonconflict.py(1 hunks)test/functional/wallet_encryption.py(1 hunks)test/functional/wallet_listdescriptors.py(3 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/support/allocators/secure.h
- doc/descriptors.md
- src/support/lockedpool.h
- src/support/lockedpool.cpp
- src/crypto/sha256_arm_shani.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/rpc/wallet.cpp
🚧 Files skipped from review as they are similar to previous changes (19)
- doc/release-notes-25158.md
- doc/release-note-26194.md
- doc/release-process.md
- src/qt/bitcoin.cpp
- test/functional/wallet_listdescriptors.py
- test/functional/wallet_encryption.py
- src/wallet/rpc/transactions.cpp
- src/wallet/spend.cpp
- test/functional/wallet_abandonconflict.py
- test/functional/p2p_disconnect_ban.py
- src/init.cpp
- src/Makefile.qt.include
- src/wallet/rpc/backup.cpp
- src/qt/askpassphrasedialog.cpp
- src/util/system.h
- test/functional/feature_block.py
- src/util/system.cpp
- src/wallet/rpc/encrypt.cpp
- test/functional/rpc_psbt.py
🧰 Additional context used
📓 Path-based instructions (1)
doc/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-27068.md
🧠 Learnings (3)
📓 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: 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#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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
doc/release-notes-27068.md (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/README.md (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to test/functional/**/*.py : Functional tests should be written in Python and placed in test/functional/
🪛 LanguageTool
doc/release-notes-27068.md
[style] ~5-~5: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...ases may now contain null characters. Prior to this change, only characters up to the ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
test/README.md
[style] ~117-~117: Consider a more concise word here.
Context: ...p` directories for the functional tests in order to speed them up. Speed-up amount varies o...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
test/README.md
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
145-145: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 733c1fdeeeaf894ceefe02c16860fe37ea1fe1f0
src/qt/askpassphrasedialog.cpp
Outdated
There was a problem hiding this comment.
ec99c2f4fa289ff004af58618fba00c6088670ca: nit: merge it into original commit maybe
4bbf5dd Detailed error message for passphrases with null chars (John Moffett) b4bdabc doc: Release notes for 27068 (John Moffett) 4b1205b Test case for passphrases with null characters (John Moffett) 00a0861 Pass all characters to SecureString including nulls (John Moffett) Pull request description: `SecureString` is a `std::string` specialization with a secure allocator. However, in practice it's treated like a C- string (no explicit length and null-terminated). This can cause unexpected and potentially insecure behavior. For instance, if a user enters a passphrase with embedded null characters (which is possible through Qt and the JSON-RPC), it will ignore any characters after the first null, potentially giving the user a false sense of security. Instead of assigning to `SecureString` via `std::string::c_str()`, assign it via a `std::string_view` of the original. This explicitly captures the size and still doesn't make any extraneous copies in memory. Note to reviewers, the following all compile identically in recent `GCC` (x86-64 and ARM64) with `-O2` (and `-std=c++17`): ```C++ std::string orig_string; std::cin >> orig_string; SecureString s; s.reserve(100); // The following all compile identically s = orig_string; s = std::string_view{orig_string}; s.assign(std::string_view{orig_string}); s.assign(orig_string.data(), orig_string.size()); ``` So it's largely a matter of preference. However, one thing to keep in mind is that we want to avoid making unnecessary copies of any sensitive data in memory. Something like `SecureString s{orig_string};` is still invalid and probably unwanted in our case, since it'd get treated as a short string and optimized away from the secure allocator. I presume that's the reason for the `reserve()` calls. Fixes bitcoin#27067. ACKs for top commit: achow101: re-ACK 4bbf5dd stickies-v: re-ACK [4bbf5dd](bitcoin@4bbf5dd) furszy: utACK 4bbf5dd Tree-SHA512: 47a96905a82ca674b18076a20a388123beedf70e9de73e42574ea68afbb434734e56021835dd9b148cdbf61709926b487cc95e9021d9bc534a7c93b3e143d2f7 Co-authored-by: Andrew Chow <github@achow101.com> fix: 27068 v21 -> v23
…category Co-authored-by: MacroFake <falke.marco@gmail.com> Co-authored-by: Claude Code <claude@anthropic.com>
… 0-fee Co-authored-by: Andrew Chow <github@achow101.com> Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: fanquake <fanquake@gmail.com>
…unction Co-authored-by: Andrew Chow <github@achow101.com>
…istdescriptors` and `importdescriptors` Co-authored-by: Andrew Chow <github@achow101.com>
PastaPastaPasta
force-pushed
the
backport-batch-jul-24-2025
branch
from
733c1fd to
49a05e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/README.md (4)
115-118: Trim filler phrasing in the intro sentence.The phrase “in order to” is verbose. A tighter alternative improves readability without losing meaning.
-If you have available RAM on your system you can create a RAM disk to use as the `cache` and `tmp` directories for the functional tests in order to speed them up. +If you have available RAM you can create a RAM disk for the tests’ `cache` and `tmp` directories to speed them up.
120-122: Use a real heading instead of bold text – resolves MD036.Bold text is flagged by markdown-lint as “emphasis used instead of a heading”.
Turn it into a sub-heading for proper structure and easier linking.-**Linux** +##### Linux
122-127: Minor fix: capital-case the size unit for consistency withmount(8).Most
mountexamples (andmountitself) expect the size suffix in upper-case (4G).
Lower-case works on many systems but fails on some older util-linux builds.-sudo mount -t tmpfs -o size=4g tmpfs /mnt/tmp/ +sudo mount -t tmpfs -o size=4G tmpfs /mnt/tmp/
145-148: macOS section: promote the label to a heading – same MD036 issue.-**macOS** +##### macOS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 733c1fdeeeaf894ceefe02c16860fe37ea1fe1f0 and 49a05e4.
📒 Files selected for processing (16)
doc/release-note-26194.md(1 hunks)doc/release-notes-27068.md(1 hunks)src/init.cpp(1 hunks)src/qt/askpassphrasedialog.cpp(3 hunks)src/qt/bitcoin.cpp(1 hunks)src/support/allocators/secure.h(1 hunks)src/util/system.cpp(5 hunks)src/util/system.h(2 hunks)src/wallet/rpc/backup.cpp(2 hunks)src/wallet/rpc/encrypt.cpp(4 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)test/README.md(1 hunks)test/functional/rpc_psbt.py(1 hunks)test/functional/wallet_encryption.py(1 hunks)test/functional/wallet_listdescriptors.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/support/allocators/secure.h
🚧 Files skipped from review as they are similar to previous changes (13)
- doc/release-note-26194.md
- src/qt/bitcoin.cpp
- test/functional/wallet_listdescriptors.py
- src/util/system.h
- src/wallet/rpc/wallet.cpp
- src/init.cpp
- test/functional/wallet_encryption.py
- src/wallet/spend.cpp
- src/wallet/rpc/backup.cpp
- test/functional/rpc_psbt.py
- src/wallet/rpc/encrypt.cpp
- src/util/system.cpp
- src/qt/askpassphrasedialog.cpp
🧰 Additional context used
📓 Path-based instructions (1)
doc/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-27068.md
🧠 Learnings (3)
📓 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: 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/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/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#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
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
doc/release-notes-27068.md (1)
Learnt from: kwvg
PR: #6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The GetWallet() function calls in src/wallet/rpcwallet.cpp are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
test/README.md (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to test/functional/**/*.py : Functional tests should be written in Python and placed in test/functional/
🪛 LanguageTool
doc/release-notes-27068.md
[style] ~5-~5: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...ases may now contain null characters. Prior to this change, only characters up to the ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
test/README.md
[style] ~117-~117: Consider a more concise word here.
Context: ...p` directories for the functional tests in order to speed them up. Speed-up amount varies o...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
test/README.md
120-120: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
145-145: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
doc/release-notes-27068.md (1)
4-6: Fix minor grammar and tighten wordingThe phrase “Prior to” is verbose, and the singular/plural agreement in “null character were” is incorrect.
[ suggest_nitpick ]- Prior to this change, only characters up to the first - null character were recognized and accepted. (#27068) + Before this change, only the characters preceding the first + null character were recognized and accepted. (#27068)
| newpass1.assign(ui->passEdit2->text().toStdString().c_str()); | ||
| newpass2.assign(ui->passEdit3->text().toStdString().c_str()); | ||
|
|
||
| oldpass.assign(std::string_view{ui->passEdit1->text().toStdString()}); |
There was a problem hiding this comment.
probably 27068 is not full, see upgradetohd in src/wallet/rpc/wallet.cpp
} else {
// TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
// Alternately, find a way to make request.params[0] mlock()'d to begin with.
secureWalletPassphrase = request.params[2].get_str().c_str();
}
|
|
||
| static bool InitSettings() | ||
| { | ||
| gArgs.EnsureDataDir(); |
There was a problem hiding this comment.
27073: need to pull 27176 as well (related docs update)
…DirBase don't create datadir c5b325a Merge bitcoin#27176: docs: GetDataDirNet and GetDataDirBase don't create datadir (fanquake) Pull request description: ## Issue being fixed or feature implemented Since bitcoin#27073 (backported via #6780), the behaviour of `GetDataDir()` [changed](https://github.com/bitcoin/bitcoin/pull/27073/files#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216L435-L443) to only return the datadir path, but not create it if non-existent. This also changed the behaviour of `GetDataDirNet()` and `GetDataDirBase()` but the docs do not yet reflect that. ## How Has This Been Tested? N/A ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK c5b325a PastaPastaPasta: utACK c5b325a Tree-SHA512: ff3fa0daea16ddc2d4140e65b7a02926fa2be9adebd6f756868ca985c49a9963950ba1ced1117b3ca289cdda6c7d44e72f51158ced1d3abb5e2f965b81f00896
Replace Bitcoin Core PR numbers (>7000) with their corresponding Dash Core backport PR numbers in release notes: - bitcoin#24098 -> dashpay#6784 (REST API endpoints) - bitcoin#22650 -> dashpay#6569 (deprecatedrpc=addresses removal) - bitcoin#22918 -> dashpay#6577 (getblock verbosity level 3) - bitcoin#23093 -> dashpay#6577 (newkeypool RPC) - bitcoin#24629 -> dashpay#6784 (pruneblockchain fix) - bitcoin#25158 -> dashpay#6780 (abandoned field in wallet RPCs)
4 participants
Issue being fixed or feature implemented
Batch of backports by @DashCoreAutoGuix
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.