backport: trivial batch 380 #6779
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis set of changes primarily removes the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Complexity label: Moderate Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (33)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (25)
⏰ 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). (12)
✨ 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 (4)
src/qt/optionsmodel.cpp (1)
408-411: Consider marking trivial helperconstexpr
GetDefaultProxyAddress()is a pure, compile-time-deterministic string constructor. Addingconstexpr(and possiblynoexcept) would enable constant-folding and avoid any run-time overhead when this value is used in other constant expressions:-static QString GetDefaultProxyAddress() +static constexpr QString GetDefaultProxyAddress() noexceptNot blocking, but brings this utility in line with modern C++20 guidelines.
src/arith_uint256.h (1)
61-212: Const-return removal is fine, but postfix operators now allow useless mutation of temporariesThe switch from
const base_uint→base_uintmatches the new clang-tidy guideline and removes an obsolete qualifier.
However, postfixoperator++/--now return a mutable prvalue, which makes expressions such as(x++) += 5;well-formed but semantically pointless (the mutation is discarded). To retain the historical safety net, consider keeping only those two postfix overloads returningconst base_uintwhile leaving the rest mutable:- base_uint operator++(int) + const base_uint operator++(int) ... - base_uint operator--(int) + const base_uint operator--(int)Not critical, but preserves the common STL behaviour and avoids accidental write-to-temporary patterns.
src/util/fees.cpp (1)
52-55: Const removal aligns with new style; consider[[nodiscard]]While updating the signature, tagging the function
[[nodiscard]]would prevent callers from ignoring the generated error string, reducing the risk of silent failures:-std::string InvalidEstimateModeErrorMessage() +[[nodiscard]] std::string InvalidEstimateModeErrorMessage()Low-priority enhancement.
src/wallet/scriptpubkeyman.h (1)
612-613: Consider marking these getters[[nodiscard]]Now that the return types are movable, callers may accidentally ignore the (potentially costly) copies. A light-weight safeguard is to add
[[nodiscard]]:- WalletDescriptor GetWalletDescriptor() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); - std::vector<CScript> GetScriptPubKeys() const; + [[nodiscard]] WalletDescriptor GetWalletDescriptor() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + [[nodiscard]] std::vector<CScript> GetScriptPubKeys() const;Purely optional, but it tightens up the API without behaviour change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
ci/test/00_setup_env_native_qt5.sh(1 hunks)src/.clang-tidy(1 hunks)src/arith_uint256.h(4 hunks)src/bench/gcs_filter.cpp(1 hunks)src/core_write.cpp(1 hunks)src/init.cpp(1 hunks)src/llmq/quorums.cpp(1 hunks)src/qt/optionsmodel.cpp(2 hunks)src/qt/rpcconsole.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/net.cpp(6 hunks)src/rpc/rawtransaction.cpp(2 hunks)src/rpc/server_util.cpp(1 hunks)src/rpc/server_util.h(2 hunks)src/script/descriptor.cpp(1 hunks)src/script/descriptor.h(1 hunks)src/streams.h(0 hunks)src/test/fuzz/util.h(1 hunks)src/test/serialize_tests.cpp(1 hunks)src/test/versionbits_tests.cpp(1 hunks)src/txmempool.cpp(1 hunks)src/txmempool.h(1 hunks)src/util/fees.cpp(1 hunks)src/util/fees.h(1 hunks)src/util/system.cpp(2 hunks)src/util/system.h(1 hunks)src/validationinterface.cpp(1 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/scriptpubkeyman.h(2 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)src/wallet/wallet.h(1 hunks)src/wallet/wallettool.cpp(1 hunks)test/functional/feature_config_args.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/streams.h
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/core_write.cppsrc/txmempool.cppsrc/rpc/server_util.hsrc/util/system.cppsrc/rpc/blockchain.cppsrc/qt/rpcconsole.cppsrc/util/system.hsrc/wallet/scriptpubkeyman.hsrc/script/descriptor.hsrc/wallet/test/wallet_tests.cppsrc/script/descriptor.cppsrc/validationinterface.cppsrc/rpc/net.cppsrc/rpc/rawtransaction.cppsrc/llmq/quorums.cppsrc/util/fees.hsrc/test/serialize_tests.cppsrc/util/fees.cppsrc/txmempool.hsrc/test/fuzz/util.hsrc/wallet/rpc/transactions.cppsrc/qt/optionsmodel.cppsrc/bench/gcs_filter.cppsrc/test/versionbits_tests.cppsrc/wallet/wallettool.cppsrc/rpc/server_util.cppsrc/wallet/scriptpubkeyman.cppsrc/arith_uint256.hsrc/wallet/wallet.hsrc/init.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/feature_config_args.py
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.cppsrc/test/serialize_tests.cppsrc/test/fuzz/util.hsrc/test/versionbits_tests.cpp
ci/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the ci directory (continuous integration)
Files:
ci/test/00_setup_env_native_qt5.sh
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/gcs_filter.cpp
🧠 Learnings (20)
📓 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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
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: kwvg
PR: dashpay/dash#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/core_write.cpp (3)
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.
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/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
src/rpc/server_util.h (2)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/rpc/blockchain.cpp (3)
Learnt from: kwvg
PR: #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: #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.
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/qt/rpcconsole.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/wallet/scriptpubkeyman.h (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.
test/functional/feature_config_args.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/wallet/test/wallet_tests.cpp (3)
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/{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: #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/rpc/net.cpp (3)
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/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/rpc/rawtransaction.cpp (2)
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.
Learnt from: kwvg
PR: #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.
src/.clang-tidy (2)
Learnt from: kwvg
PR: #6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with // clang-format off and // clang-format on directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Learnt from: kwvg
PR: #6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use // clang-format off and // clang-format on directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
src/llmq/quorums.cpp (1)
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
src/test/serialize_tests.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/test/fuzz/util.h (1)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/fuzz/**/*.{cpp,h,cc,cxx,hpp} : Fuzzing harnesses should be placed in src/fuzz/
src/wallet/rpc/transactions.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/test/versionbits_tests.cpp (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/wallettool.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/rpc/server_util.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: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
src/wallet/scriptpubkeyman.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/wallet/wallet.h (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 (4)
test/functional/feature_config_args.py (1)
test/functional/test_framework/test_framework.py (1)
start_node(639-648)
src/script/descriptor.h (1)
src/script/descriptor.cpp (6)
GetCachedParentExtPubKeys(1256-1259)GetCachedParentExtPubKeys(1256-1256)GetCachedDerivedExtPubKeys(1261-1264)GetCachedDerivedExtPubKeys(1261-1261)GetCachedLastHardenedExtPubKeys(1266-1269)GetCachedLastHardenedExtPubKeys(1266-1266)
src/util/fees.h (1)
src/util/fees.cpp (2)
InvalidEstimateModeErrorMessage(52-55)InvalidEstimateModeErrorMessage(52-52)
src/txmempool.h (1)
src/txmempool.cpp (2)
RemovalReasonToString(1742-1753)RemovalReasonToString(1742-1742)
⏰ 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_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (44)
src/wallet/rpc/transactions.cpp (1)
419-419: LGTM: Removal of unnecessary const qualifier from return type.This change correctly removes the
constqualifier from the return type ofTransactionDescriptionString(). Returningconstobjects by value is unnecessary and can prevent move optimizations. The change aligns with modern C++ best practices and the broader cleanup pattern in this PR.src/test/fuzz/util.h (1)
118-118: Consistent with cleanup pattern, minor style change.The removal of
constfrom the local variableszand the change to brace initialization{}aligns with the broader cleanup in this PR. While removingconstfrom an unmodified local variable slightly reduces type safety, it's consistent with the overall pattern of removing unnecessary const qualifiers.src/bench/gcs_filter.cpp (1)
8-8: LGTM: Removal of unnecessary const qualifier from return type.The removal of
constfrom the return type ofGenerateGCSTestElements()is correct. Returningconstobjects by value provides no benefit and can hinder move optimizations. This change aligns with the broader cleanup pattern in this PR.src/test/versionbits_tests.cpp (1)
18-18: LGTM: Removal of unnecessary const qualifier from return type.The removal of
constfrom the return type ofStateName()is appropriate. Returningconst std::stringby value is unnecessary and can prevent move optimizations. This change maintains the same functionality while aligning with modern C++ practices and the cleanup pattern in this PR.src/validationinterface.cpp (1)
19-19: LGTM: Removal of unnecessary const qualifier from return type.The removal of
constfrom the return type of theRemovalReasonToString()function declaration is consistent with the broader cleanup pattern in this PR. Returningconst std::stringby value provides no benefit and can hinder move optimizations.src/qt/optionsmodel.cpp (1)
36-36: Redundantconstqualifier correctly removedReturning a
QStringby value already yields an r-value that callers cannot accidentally modify in-place; the priorconstprovided no additional safety while hindering move semantics. ✔️ci/test/00_setup_env_native_qt5.sh (1)
17-17: Verify thatNO_WERROR=1is actually respectedIf the build still passes
-Werrorunconditionally from other layers (e.g../configurecache, ccache wrapper, or CI job config), this new env-var may have no effect, leaving CI susceptible to unrelated warnings.Would you confirm that
NO_WERRORis consumed by the top-levelconfigureor the wrapper scripts on this CI path?src/.clang-tidy (1)
5-6: Good addition ofreadability-const-return-typecheckEnabling this check consistently enforces the style change project-wide. No further action required.
src/util/fees.h (1)
16-16: LGTM! Removal of redundant const qualifier on return type.The
constqualifier on a return-by-value type is redundant since the caller receives a copy. This cleanup improves consistency with modern C++ practices.src/txmempool.h (1)
359-359: LGTM! Consistent cleanup of redundant const qualifier.The removal of
constfrom the return type aligns with the broader cleanup pattern in this PR and is correct for return-by-value functions. The implementation insrc/txmempool.cppconfirms this returns string values, making the const qualifier redundant.src/wallet/test/wallet_tests.cpp (1)
57-57: LGTM! Consistent removal of redundant const qualifier in test utility.The
constqualifier on thestd::shared_ptr<CWallet>return type is redundant since the function returns by value. This cleanup aligns with the broader pattern of const qualifier removal throughout the codebase and maintains consistency in the test utilities.src/llmq/quorums.cpp (1)
746-747: LGTM: Consistent with CDataStream constructor changes.The change from constructor-based serialization to explicit streaming using the
<<operator aligns with the broader codebase update that removed the variadic template constructor fromCDataStream. The functionality remains equivalent - the same data (requestandbody) is serialized in the same order.src/test/serialize_tests.cpp (1)
292-293: LGTM: Test correctly adapted to CDataStream constructor changes.The test has been properly updated to use explicit serialization with the
<<operator instead of the removed variadic constructor. The test logic remains functionally equivalent - it still validates that the serialization and deserialization process works correctly for the test objects.src/core_write.cpp (1)
183-184: LGTM: Good defensive programming with verbosity validation.The addition of
CHECK_NONFATAL(verbosity >= TxVerbosity::SHOW_DETAILS)provides a runtime contract validation that ensures the function is called with an appropriate verbosity level. This aligns with the broader changes where RPC functions gained verbosity parameters, and helps catch any incorrect usage early in development.src/wallet/wallet.h (1)
962-967: Good catch—returning by value eliminates a dangling-reference hazardThe old signature most likely returned
const std::string&that referenced a temporary produced bystrprintf, yielding UB. Returning by value is the correct and inexpensive fix (NRVO takes care of copies). Implementation looks solid.src/init.cpp (1)
761-761: LGTM! Correctly removes unnecessary sensitivity marking.The removal of
ArgsManager::SENSITIVEfrom the-rpcbindargument registration is appropriate. The-rpcbindargument contains network binding addresses (IP:port), which are configuration details rather than sensitive credentials like passwords. This change aligns with the corresponding test updates that expect these arguments to be logged without masking.src/txmempool.cpp (1)
1742-1742: LGTM: Correct removal of unnecessary const qualifier.Removing
constfrom the return type of a function returning an object by value is appropriate, as the const qualifier provides no benefit when the caller receives a copy of the returned object.src/rpc/server_util.h (2)
16-16: LGTM: Appropriate forward declaration.Adding the forward declaration for
BanManfollows the established pattern for other classes in this header file.
25-26: LGTM: Consistent utility function declarations.The new
EnsureBanmanandEnsureAnyBanmanfunction declarations follow the established naming convention and signature pattern of other utility functions in this file, providing standardized access to theBanManinstance.test/functional/feature_config_args.py (2)
135-137: LGTM: Test correctly updated to reflect logging behavior change.The test properly validates that
rpcbindandrpcallowiparguments are no longer masked in debug logs by adding them to theunexpected_msgslist, which aligns with the change to remove theSENSITIVEflag from these arguments.
142-144: LGTM: Additional test arguments support the logging validation.The extra
-rpcbindand-rpcallowiparguments provide the necessary input to validate that these arguments are no longer treated as sensitive in the logging output.src/wallet/wallettool.cpp (1)
60-60: LGTM: Correct removal of unnecessary const qualifier.Removing
constfrom the return type of a function returningstd::shared_ptrby value is appropriate, as the const qualifier provides no benefit when the caller receives a copy of the smart pointer.src/qt/rpcconsole.cpp (1)
829-830: LGTM - Improves wallet selection consistencyThe unconditional selection of the first wallet when added aligns the GUI behavior with wallet RPC expectations and removes unnecessary complexity from the previous visibility-based logic.
src/rpc/net.cpp (4)
744-744: LGTM - Standardizes BanMan access patternUsing
EnsureBanman(node)instead of directnode.banmanaccess improves error handling consistency and follows the established pattern for safely accessing node subsystems in RPC commands.
767-767: LGTM - Consistent use of local BanMan referenceThe calls correctly use the local
banmanreference obtained fromEnsureBanman(), maintaining the same functionality while improving code consistency.Also applies to: 780-780, 785-785, 793-793
824-824: LGTM - Consistent BanMan helper usageProperly uses
EnsureAnyBanman(request.context)and the local reference forGetBanned(), following the same refactoring pattern as other ban-related RPC commands.Also applies to: 827-827
861-861: LGTM - Completes consistent BanMan refactoringBoth
clearbannedandcleardiscouragedcommands now follow the same pattern as other ban-related RPC commands, usingEnsureAnyBanman()and local references for method calls.Also applies to: 863-863, 882-882, 884-884
src/rpc/server_util.cpp (2)
60-66: LGTM! Function follows established patterns correctly.The
EnsureBanmanfunction correctly implements the standard pattern for accessing node subsystems, with appropriate error handling and return type consistency.
68-71: LGTM! Consistent with existing "EnsureAny" pattern.The
EnsureAnyBanmanfunction correctly follows the established pattern of delegating toEnsureAnyNodeContextand then calling the correspondingEnsurefunction.src/util/system.h (2)
268-268: Good improvement! Removed unnecessary const qualifier from return type.Removing
constfrom the return type ofGetUnsuitableSectionOnlyArgs()follows modern C++ best practices. Const qualifiers on return-by-value types are unnecessary and can prevent move optimizations.
273-273: Good improvement! Removed unnecessary const qualifier from return type.Removing
constfrom the return type ofGetUnrecognizedSections()follows modern C++ best practices and is consistent with the other function signature improvements in this file.src/util/system.cpp (2)
271-271: Correct implementation change to match header declaration.The removal of
constfrom the return type ofGetUnsuitableSectionOnlyArgs()correctly matches the header file change while preserving all existing functionality.
291-291: Correct implementation change to match header declaration.The removal of
constfrom the return type ofGetUnrecognizedSections()correctly matches the header file change while preserving all existing functionality and filtering logic.src/wallet/scriptpubkeyman.cpp (2)
2462-2465: Return-type const removal is harmless; implementation looks correctReturning by value means the caller gets a copy, so the previous
constqualifier was redundant. The change therefore only simplifies the signature without affecting behaviour. No further action required.
2467-2477: Implementation is straightforward and efficientThe helper reserves capacity up-front, iterates once, and remains correctly guarded by
cs_desc_man. No correctness or performance concerns detected.src/rpc/blockchain.cpp (1)
85-85: LGTM: Backward-compatible function signature extensionThe addition of the optional
TxVerbosity verbosityparameter with a default value maintains backward compatibility while enabling dynamic control over transaction serialization verbosity. This change aligns with the coordinated updates across the transaction JSON conversion workflow.src/script/descriptor.cpp (3)
1256-1259: LGTM: Correctly removes redundant const qualifierThe
constqualifier on return types of functions that return by value is unnecessary since the caller receives a copy anyway. This change aligns with modern C++ best practices.
1261-1264: LGTM: Correctly removes redundant const qualifierSame as above - the
constqualifier on the return type is unnecessary when returning by value.
1266-1269: LGTM: Correctly removes redundant const qualifierConsistent with the other methods - removing the redundant
constqualifier from the return type when returning by value.src/rpc/rawtransaction.cpp (3)
69-69: LGTM: Backward-compatible function signature enhancement.The addition of the optional
TxVerbosity verbosityparameter with a default value ofTxVerbosity::SHOW_DETAILSmaintains backward compatibility while enabling verbosity control for transaction JSON serialization.
71-71: Good defensive programming practice.The non-fatal assertion ensures that the verbosity level is at least
SHOW_DETAILS, providing runtime validation of the parameter's expected range. This aligns with the corresponding assertion mentioned in the AI summary forTxToUnivinsrc/core_write.cpp.
105-105: Correct parameter forwarding.The
TxToUnivcall properly forwards the verbosity parameter, enabling dynamic control over transaction serialization verbosity as intended by this enhancement.src/script/descriptor.h (1)
69-73: Excellent: Removal of unnecessary const qualifiers on return types.The removal of
constqualifiers from the return types of these getter methods is a proper C++ modernization. Since these methods return by value (copies of the internal maps), theconstqualifier on the return type was unnecessary and redundant. The methods themselves correctly remainconstmember functions, preserving the immutability contract for the class instance.This change aligns with modern C++ best practices and is consistent with the trivial nature of this backport batch.
src/wallet/scriptpubkeyman.h (1)
37-37: Un‐const’ing the by-value return enables move semantics—LGTMTop-level
conston a by-value return was meaningless and inhibited move operations. Dropping it is the correct fix and is ABI-neutral.
|
This pull request has conflicts, please rebase. |
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
backport-0.23-batch-380
branch
from
c74ec49 to
64304ec
Compare
* Merge bitcoin#27069: net: add `Ensure{any}Banman` 2d955ff net: add `Ensure{any}Banman` (brunoerg) Pull request description: This PR adds `Ensure{any}Banman` functions to avoid code repetition and make it cleaner. Same approach as done with argsman, chainman, connman and others. ACKs for top commit: davidgumberg: ACK [2d955ff](bitcoin@2d955ff) Tree-SHA512: 0beb7125312168a3df130c1793a1412ab423ef0f46023bfe2a121630c79df7e55d3d143fcf053bd09e2d96e9385a7a04594635da3e5c6be0c5d3a9cafbe3b631 * Apply suggestions from code review --------- Co-authored-by: merge-script <[email protected]>
…y constructor Co-authored-by: Claude Code <[email protected]>
576f7b8 Fix misleading RPC console wallet message (John Moffett) Pull request description: ## Misleading message from RPCConsole window ## In certain circumstances, the GUI console will display the message 'Executing command without any wallet' when it is, in fact, using the currently loaded wallet. For instance:  In RPC calls, if no wallet is explicitly selected and there is exactly one wallet loaded, the [default](https://github.com/bitcoin-core/gui/blob/39363a4b945114f5e4718f75098f3036e8fe6a1d/src/wallet/rpc/util.cpp#L71-L93) is to act on that loaded wallet. The GUI console acts that way in reality, but sometimes erroneously reports that it's not acting on any particular wallet. The root issue is due to the logic that prevents changing the selected wallet if the RPCConsole is visible: https://github.com/bitcoin-core/gui/blob/39363a4b945114f5e4718f75098f3036e8fe6a1d/src/qt/rpcconsole.cpp#L783-L786 This PR removes that unnecessary logic. This does have some ramifications. Prior to this PR, if a user opened the console window without any wallets loaded, then opened two or more wallets, the RPC console would select "None" of the wallets and any wallet-specific RPCs would fail. However, the behavior was different if the user hadn't had the console window open. In that case, if they opened the RPC Console window _after_ loading at least the first wallet, it would select the first-loaded wallet. This context-dependent behavior is (IMO) undesirable, and this PR changes it to be consistent. ACKs for top commit: hebasto: ACK 576f7b8, tested on Ubuntu 22.04 (Qt 5.15.3). Tree-SHA512: 627da186025ba4f4e8df7fdd1b10363f923c4ecc50f023bbf2aece6e2593da65c45147c933effaca9040f813a6e46f034fc2d1ee2fb0f401000a3a6221a0e36e Co-authored-by: Hennadii Stepanov <[email protected]>
PastaPastaPasta
force-pushed
the
backport-0.23-batch-380
branch
from
64304ec to
1019c65
Compare
…n-type violations
fa451d4 Fix clang-tidy readability-const-return-type violations (MarcoFalke)
Pull request description:
This comes up during review, so instead of wasting review cycles on this, just enforce it via CI
ACKs for top commit:
stickies-v:
utACK fa451d4
hebasto:
ACK fa451d4.
… SHOW_DETAILS a24e633 refactor: rpc: set TxToJSON default verbosity to SHOW_DETAILS (stickies-v) Pull request description: and are only to be called when we want to decode the transaction (i.e. its details) into JSON. If is , the function should not have been (and currently is not) called in the first place. There is no behaviour change, current logic simply assumes anything less than equals . With this change, the assumptions and intent become more explicit. ACKs for top commit: w0xlt: ACK bitcoin@a24e633 Tree-SHA512: b97235adae49b972bdbe10aca1438643fb35ec66a4e57166b1975b3015bc5a06a711feebe4453a8fefe71781e484b21ef80847d8e8a33694a3abcc863accd4d7 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>
…bind b9d5674 init: Remove sensitive flag from rpcbind (Andrew Chow) Pull request description: `-rpcbind` is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if `rpcbind` is actually activated, the bound IP addresses will be written to the log anyways. The test `feature_config_args.py` did not catch this contradiction as the test node was not started with `rpcallowip` and so `rpcbind` was not acted upon. This also brings `rpcbind` inline with `bind` as that is not flagged as sensitive either. ACKs for top commit: Sjors: re-utACK b9d5674 willcl-ark: ACK b9d5674 theStack: ACK b9d5674 Tree-SHA512: 50ab5ad2e18ae70649deb1ac429d404b5f5c41f32a4943b2041480580152df22e72d4aae493379d0b23fcb649ab342376a82119760fbf6dfdcda659ffd3e244a Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^[email protected]>
Add missing banman.h include for EnsureBanman functions and fix SetupDescriptorScriptPubKeyMans call with required parameters. Fixes compilation failures introduced in 1c0a3d0 (Merge bitcoin#27069: net: add Ensure{any}Banman).
PastaPastaPasta
force-pushed
the
backport-0.23-batch-380
branch
from
1019c65 to
bf27280
Compare
3 participants
Issue being fixed or feature implemented
Batch of trivial backports
What was done?
See commits
How Has This Been Tested?
CI
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.