backport: bitcoin#24419, #25351, #25466, #25701, #25707, #25733, #25872 #6980
Conversation
|
WalkthroughThis PR applies widespread small refactors and CI/linters updates: many local bindings changed from by-value to const-ref (and a few to non-const ref to enable moves), some includes removed, UniValue container APIs changed to take and move values (header + implementation), DescriptorScriptPubKeyMan AddKey/AddCryptedKey signatures extended, wallet rescan now scans the mempool when no max_height is provided, multiple RPC help texts updated to mention mempool, tests adjusted to reflect mempool-inclusive rescans and UniValue API changes, and CI lint script now processes src/util/string.cpp. The clang-tidy configuration is expanded with additional checks and one CheckOptions entry. Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Wallet as ScanForWalletTransactions
participant Chain as BlockIterator
participant Mempool as MemPoolScanner
Caller->>Wallet: ScanForWalletTransactions(max_height=null)
Wallet->>Chain: Iterate blocks (up to tip)
Chain-->>Wallet: Blocks scanned
alt max_height provided
Wallet-->>Caller: Finish (no mempool scan)
else max_height null
Wallet->>Mempool: WITH_LOCK: Scan mempool transactions
Mempool-->>Wallet: Mempool scanned
Wallet-->>Caller: Finish (blocks + mempool)
end
sequenceDiagram
participant Caller as Call Site
participant UniValue as UniValue
participant Storage as Internal Storage
Caller->>UniValue: pushKV(std::string key, UniValue val) -- now by-value
Note right of UniValue: key and val are moved into UniValue
UniValue->>Storage: storage.emplace(std::move(key), std::move(val))
UniValue-->>Caller: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas that may need extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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 (1)
src/wallet/walletdb.cpp (1)
887-891: Use const-ref in for-range to avoid copies — good change.No behavior change; reduces allocations when iterating descriptor caches/keys.
Also applies to: 894-902, 904-912
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
ci/dash/lint-tidy.sh(1 hunks)src/.clang-tidy(1 hunks)src/bench/wallet_loading.cpp(0 hunks)src/bitcoin-cli.cpp(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/blockfilter.cpp(1 hunks)src/coins.cpp(1 hunks)src/core_read.cpp(1 hunks)src/init.cpp(1 hunks)src/logging.cpp(1 hunks)src/netbase.cpp(3 hunks)src/node/blockstorage.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/output_script.cpp(0 hunks)src/rpc/rawtransaction_util.cpp(1 hunks)src/rpc/util.cpp(1 hunks)src/script/descriptor.cpp(2 hunks)src/test/base58_tests.cpp(2 hunks)src/test/blockfilter_tests.cpp(1 hunks)src/test/fuzz/txorphan.cpp(1 hunks)src/test/key_io_tests.cpp(3 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(4 hunks)src/test/util_tests.cpp(1 hunks)src/test/validation_block_tests.cpp(1 hunks)src/univalue/include/univalue.h(1 hunks)src/univalue/lib/univalue.cpp(2 hunks)src/util/message.cpp(0 hunks)src/util/strencodings.cpp(0 hunks)src/util/string.cpp(1 hunks)src/util/string.h(0 hunks)src/util/threadinterrupt.h(1 hunks)src/wallet/bdb.cpp(1 hunks)src/wallet/receive.cpp(1 hunks)src/wallet/rpc/backup.cpp(8 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(3 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/rpc_wipewallettxes.py(1 hunks)test/functional/wallet_balance.py(1 hunks)test/functional/wallet_import_rescan.py(5 hunks)test/functional/wallet_importdescriptors.py(2 hunks)test/lint/run-lint-format-strings.py(0 hunks)
💤 Files with no reviewable changes (6)
- src/util/strencodings.cpp
- src/bench/wallet_loading.cpp
- test/lint/run-lint-format-strings.py
- src/util/message.cpp
- src/util/string.h
- src/rpc/output_script.cpp
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash 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.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash 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.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/test/base58_tests.cppsrc/test/blockfilter_tests.cpptest/functional/wallet_balance.pytest/functional/wallet_importdescriptors.pysrc/wallet/wallet.cppsrc/rpc/rawtransaction_util.cpptest/functional/rpc_wipewallettxes.pysrc/wallet/rpc/backup.cppsrc/test/sighash_tests.cpptest/functional/wallet_import_rescan.pysrc/wallet/test/wallet_tests.cppsrc/univalue/include/univalue.hsrc/univalue/lib/univalue.cppsrc/test/transaction_tests.cppsrc/bitcoin-cli.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/test/base58_tests.cppsrc/bitcoin-tx.cppsrc/test/blockfilter_tests.cppsrc/rpc/rawtransaction_util.cppsrc/univalue/include/univalue.hsrc/univalue/lib/univalue.cppsrc/test/transaction_tests.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/test/base58_tests.cpptest/functional/wallet_importdescriptors.pytest/functional/rpc_wipewallettxes.pysrc/test/sighash_tests.cpptest/functional/wallet_import_rescan.pysrc/wallet/test/wallet_tests.cpp
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/test/base58_tests.cppsrc/test/transaction_tests.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/test/validation_block_tests.cppsrc/node/blockstorage.cpp
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
test/functional/wallet_balance.pytest/functional/wallet_importdescriptors.pytest/functional/rpc_wipewallettxes.py
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/wallet.cppsrc/wallet/test/wallet_tests.cppsrc/bitcoin-cli.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/wallet.cpp
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/wallet_import_rescan.py
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/netbase.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/netbase.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/netbase.cpp
🧬 Code graph analysis (8)
test/functional/wallet_balance.py (2)
src/wallet/rpc/backup.cpp (7)
descriptors(2029-2029)dumpprivkey(831-875)dumpprivkey(831-831)importaddress(207-308)importaddress(207-207)importprivkey(87-179)importprivkey(87-87)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_importdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/wallet/wallet.cpp (6)
src/node/blockstorage.cpp (2)
WITH_LOCK(518-518)WITH_LOCK(773-773)src/rpc/quorums.cpp (2)
WITH_LOCK(138-138)WITH_LOCK(1011-1011)src/rpc/rawtransaction.cpp (1)
WITH_LOCK(651-651)src/net_processing.cpp (3)
WITH_LOCK(331-334)WITH_LOCK(3168-3168)WITH_LOCK(3190-3190)src/qt/clientmodel.cpp (1)
WITH_LOCK(175-175)src/wallet/wallet.h (1)
cs_wallet(592-592)
src/rpc/rawtransaction_util.cpp (1)
src/rpc/request.cpp (2)
JSONRPCError(56-62)JSONRPCError(56-56)
test/functional/rpc_wipewallettxes.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
src/rpc/blockchain.cpp (3)
src/test/script_tests.cpp (1)
script(286-286)src/wallet/interfaces.cpp (4)
script(190-197)script(190-190)script(206-210)script(206-206)src/script/descriptor.cpp (2)
scripts(758-763)scripts(758-758)
test/functional/wallet_import_rescan.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (2)
start_nodes(660-678)sync_mempools(848-874)
src/univalue/include/univalue.h (1)
src/univalue/lib/univalue.cpp (12)
push_back(104-109)push_back(104-104)push_backV(111-116)push_backV(111-111)__pushKV(118-124)__pushKV(118-118)key(188-198)key(188-188)pushKV(126-135)pushKV(126-126)pushKVs(137-144)pushKVs(137-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-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: 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: Lint / Run linters
🔇 Additional comments (58)
src/util/threadinterrupt.h (1)
9-9: LGTM!The include addition is well-placed and necessary to support the thread-safety annotations already in use throughout the file (e.g.,
EXCLUSIVE_LOCKS_REQUIREDon lines 26 and 28). This aligns with the PR's static analysis enhancements objective.src/wallet/scriptpubkeyman.cpp (1)
1929-1929: LGTM! Efficient iteration with const reference.This change eliminates unnecessary copies during iteration by using
const auto&instead ofauto. The semantic behavior is unchanged sincekey_pairis only read within the loop body.src/test/fuzz/txorphan.cpp (1)
71-71: LGTM! Typo correction improves readability.The comment typo fix from "poped" to "popped" is correct and improves code quality.
src/coins.cpp (1)
359-359: LGTM! Performance optimization eliminates unnecessary copies.The change from by-value to const-reference binding avoids copying callback function objects during iteration, which is a beneficial performance optimization with no functional impact.
src/test/util_tests.cpp (1)
1307-1321: LGTM: NOLINT suppressions are correctly applied.The inline
NOLINT(*-use-after-move)comments appropriately silence false-positive static analysis warnings. The test intentionally uses the addresses of moved-from objects (&t2,&t3) to verify that theTracker::originpointer correctly references the original object after move operations. Taking the address of a moved-from object is safe and necessary for this test's correctness validation.src/qt/splashscreen.cpp (1)
58-58: LGTM! Valid copy-elimination optimization.Binding
titleAddTextas a const reference instead of copying the QString avoids an unnecessary allocation. All uses (lines 112, 116, 123) are read-only, and the lifetime is correctly scoped to the constructor where all references occur.src/node/blockstorage.cpp (1)
451-451: Need to verify if this matches upstream Bitcoin Core or if this is a backport divergence. Let me check for any upstream context:Binding temporary to const reference creates dangling reference.
The return type of
GetBlocksDirPath()isfs::path(by value), notconst fs::path&. Line 451 binds this temporary to a const lvalue reference:const fs::path& blocksdir = gArgs.GetBlocksDirPath();This creates a dangling reference—the temporary is destroyed at the statement's end, leaving
blocksdiras a dangling pointer used in the loop at line 452. All other callers in the codebase (init.cpp:1160, init.cpp:2321, validation.cpp:2841, qt/clientmodel.cpp:255) pass the result directly to functions, never binding to a reference.Fix: Remove the reference binding. Use
const fs::path blocksdir = gArgs.GetBlocksDirPath();(copy) or pass directly tofs::directory_iteratorwithout intermediate storage.⛔ Skipped due to learnings
Learnt from: knst Repo: dashpay/dash PR: 6692 File: src/llmq/blockprocessor.cpp:217-224 Timestamp: 2025-08-19T14:57:31.801Z Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.Learnt from: knst Repo: dashpay/dash PR: 6883 File: src/rpc/rawtransaction.cpp:1088-1125 Timestamp: 2025-10-13T12:37:12.357Z Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.Learnt from: knst Repo: dashpay/dash PR: 6916 File: src/univalue/include/univalue.h:81-88 Timestamp: 2025-10-25T07:08:51.918Z Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.src/netbase.cpp (1)
393-393: LGTM! Correct optimization.These changes eliminate unnecessary assignments to
recvrwhile preserving correct error handling. The variablerecvris properly initialized at line 441 and within the switch statement (lines 465-476) before being checked at line 479, so there are no stale value issues. This optimization reduces unnecessary variable assignments without affecting the SOCKS5 negotiation flow.Also applies to: 416-416, 482-482
src/script/descriptor.cpp (1)
550-550: LGTM - backport pattern matches upstream.The removal of explicit
std::move()calls in favor of direct concatenation aligns with the backport objectives. While this changes from move to copy semantics for these temporary strings, the changes are semantically correct and match the upstream Bitcoin Core modifications.Also applies to: 574-574
src/rpc/blockchain.cpp (1)
2530-2533: LGTM! Valid move optimization.The change from
const auto& scripttoCScript& scriptenables moving the script intodescriptorson line 2533, avoiding an unnecessary copy. The sequence is correct: the script is first copied intoneedles(line 2532), then moved intodescriptors(line 2533), which is safe since the moved-from element is never accessed again.src/wallet/receive.cpp (1)
431-431: LGTM: Valid const-reference optimization.The loop iterator now uses a const reference to avoid copying
std::set<CTxDestination>objects during iteration. This is a standard performance optimization with no behavioral changes.src/logging.cpp (1)
465-465: LGTM: Efficient const-reference binding.Binding the result of
ThreadGetInternalName()to a const reference avoids an unnecessary string copy. The temporary's lifetime is extended to the scope of the reference, making this safe.src/util/string.cpp (1)
13-13: LGTM: Correct removal of ineffective std::move.Removing
std::move(search)is correct sincesearchis aconst std::string¶meter. Moving from a const reference is ineffective, so the previous code was misleading. This change improves code clarity.ci/dash/lint-tidy.sh (1)
47-47: LGTM: Administrative linting update.Adding
src/util/string.cppto the IWYU check list is appropriate given the changes to that file in this PR.src/wallet/spend.cpp (1)
316-316: LGTM: Optimization to enable move semantics.Changing the loop variable to a non-const reference allows moving
coinobjects at line 320 (std::move(coin)) instead of copying them. SinceAvailableCoinsListUnspent(wallet).all()returns a temporary vector, iterating with non-const references and moving the elements is a valid optimization.src/wallet/bdb.cpp (1)
446-446: LGTM: Const-reference optimization for map iteration.Using a const reference when iterating over
m_databasesavoids copying map entries. This is a standard performance optimization with no behavioral changes.src/bitcoin-cli.cpp (1)
951-951: LGTM: Const-reference binding avoids string copy.Binding
wallet_nameas a const reference to the result ofwallet.get_str()avoids an unnecessary string copy. Both uses ofwallet_name(lines 952 and 954) are within the same scope, making this optimization safe.src/blockfilter.cpp (1)
173-173: LGTM: Const-reference optimization for map iteration.Using a const reference when iterating over
g_filter_typesavoids copying map entries. This is a standard performance optimization consistent with the broader changes in this PR.src/test/validation_block_tests.cpp (1)
185-191: Micro-copy elimination in drain loop looks goodBinding
blockasconst auto&avoids extrashared_ptrcopies; behavior unchanged.src/rpc/mining.cpp (1)
755-762: Const-ref bind of longpoll id is fineRemoves a redundant string copy; scope-limited use is safe.
src/init.cpp (1)
1436-1444: OK to bind datadir as const referenceRemoves one
fs::pathcopy; usage stays within scope; no behavior change.src/rpc/util.cpp (1)
100-108: ParseHashV copy-avoidance is fineConst-ref to the returned string is scoped and safe; logic unchanged.
src/test/miniscript_tests.cpp (1)
119-121: UDL import for miniscript tests is correctBringing
operator"" _mstinto scope is needed for the tests; good addition.src/core_read.cpp (1)
184-199: Sighash parsing: const-ref bind LGTMMinor copy elimination with no semantic change.
src/rpc/rawtransaction_util.cpp (1)
149-150: Avoid UniValue copies in ParsePrevoutsBinding
prevTxsandprevOutas const refs reduces allocations; behavior unchanged.Also applies to: 156-157
src/wallet/test/wallet_tests.cpp (3)
784-786: LGTM: rescan path counts two AddToWallet events.Expectation of 2 while queue is blocked looks correct and stable.
797-799: LGTM: stale notifications counted after unblocking.Expecting the second pair of events (total 4) after SyncWithValidationInterfaceQueue() is correct.
821-821: LGTM: immediate notification path counts once.Asserting 2 in the second scenario (no double-count) matches the intended timing.
src/.clang-tidy (1)
4-14: Tighter static analysis config looks good.Enabling performance/bugprone checks and gating as errors is fine; expect some churn in first run.
Please confirm lint CI passes after this change on all supported compilers.Also applies to: 15-26, 27-29
src/test/base58_tests.cpp (1)
27-27: Bind JSON test elements by const reference.Nice micro-optimization; keeps semantics intact.
Also applies to: 49-49
src/test/script_tests.cpp (1)
884-884: Const-ref bind for test element — good.Avoids UniValue copies; no logic changes.
src/bitcoin-tx.cpp (1)
558-606: Const-ref prevOut in sign loop — correct and efficient.No behavior change; keeps validations identical while avoiding copies.
src/test/blockfilter_tests.cpp (1)
147-147: Const-ref bind for test entry.Removes needless UniValue copies; fine.
src/test/sighash_tests.cpp (1)
165-165: Const-ref bind for sighash JSON test entry.Good small cleanup; keeps behavior identical.
src/test/key_io_tests.cpp (1)
31-31: LGTM! Efficient const-reference bindings eliminate unnecessary copies.The conversion from by-value to const-reference for test loop variables is a clean performance optimization that avoids copying UniValue objects while preserving read-only access semantics.
Also applies to: 89-89, 129-129
src/test/transaction_tests.cpp (2)
156-156: LGTM! Efficient const-reference bindings for test variables.The conversion avoids unnecessary UniValue copies while maintaining read-only access within the test loops.
Also applies to: 240-240
175-175: LGTM! Safe const-reference binding to get_array() result.Binding a const reference to the result of
get_array()is safe here—either it aliases an existing reference (ifget_array()returns by reference) or extends the lifetime of a temporary (if it returns by value). Usage is read-only within the loop scope.Also applies to: 259-259
src/univalue/lib/univalue.cpp (4)
104-109: LGTM! Move-semantics optimization for push_back.The signature change to pass
UniValueby value enables move semantics: callers passing rvalues incur no copy, while lvalues are copied into the parameter, then the parameter is moved into storage. This is more efficient than the previous const-reference approach which always copied.
118-124: LGTM! Move-semantics optimization for __pushKV.Both key and value parameters are now passed by value and moved into internal storage, enabling efficient handling of rvalue arguments while maintaining correctness for lvalues.
126-135: LGTM! Move-semantics optimization for pushKV with correct conditional logic.The method correctly handles key replacement (line 132) and new key insertion (line 134) using move semantics. When replacing an existing key's value, the new value is moved into place; when inserting a new key-value pair, both are moved to
__pushKV.
137-144: LGTM! Move-semantics optimization for pushKVs.The parameter is passed by value and its keys/values are moved into the target object within the loop. This efficiently transfers ownership from the source object.
src/univalue/include/univalue.h (1)
80-80: ✓ Public API move semantics implementation verified and correct.The method signatures at lines 80 and 85–87 have been properly updated from const-reference to by-value parameters. The implementation (univalue.cpp line 104) correctly uses
std::move(val)to forward the parameter, enabling move semantics optimization for rvalues while maintaining backward compatibility for lvalues through implicit copy.The extensive call sites throughout the codebase (700+ occurrences) all work correctly with the new signatures. This is a standard C++11 optimization pattern with no breaking changes.
test/functional/rpc_wipewallettxes.py (1)
36-37: Test change correctly implements mempool-aware rescan behavior from Bitcoin Core bitcoin#25351 backport.The assertion change from 103 to 104 reflects the upstream Bitcoin Core bitcoin#25351 backport which implements "Rescan mempool for transactions as well." After
wipewallettxes(True)removes the unconfirmed transaction from the wallet,rescanblockchain()now recovers it from the mempool, correctly resulting in 104 transactions instead of 103. This change matches the exact modifications in the upstream backport commit.test/functional/wallet_balance.py (1)
283-305: LGTM: Mempool-aware import test correctly validates new behavior.The test appropriately verifies that mempool transactions are recognized during import operations:
- After
importaddress, the mempool transaction appears inwatchonly.untrusted_pending- After
importprivkey, the transaction moves tomine.untrusted_pendingThe conditional check at lines 298-299 with the explanatory comment is acceptable for this backport.
src/wallet/wallet.cpp (2)
1688-1689: LGTM: Documentation accurately describes mempool scanning behavior.The updated documentation clearly states that mempool will be scanned when
max_heightis not set, which aligns with the implementation.
1810-1813: LGTM: Mempool scanning implementation is correct and thread-safe.The implementation correctly:
- Scans mempool only when
max_heightis not set (full rescan scenario)- Uses
WITH_LOCK(cs_wallet, ...)for thread safety, consistent with patterns seen elsewhere in the codebase- Executes after block scanning completes, ensuring proper ordering
test/functional/wallet_importdescriptors.py (2)
430-431: LGTM: Transaction ID now captured for mempool verification.Storing
send_txidenables the subsequent assertions at lines 458-459 to verify mempool transaction visibility after descriptor import.
457-459: Rewritten Review Comment: Remove incorrect explanation of address derivationThe assertions at lines 457-459 correctly verify that mempool transactions are recognized after importing descriptors to a watch-only wallet:
- The transaction from
wmulti_privexists in the mempool- The transaction is correctly included in
wmulti_pub.listunspent(0)(unconfirmed outputs)However, the change address at line 457 (
'8y2sLiPQnB81bAeiRvwbjozJXnCCNH2nHb') is different from line 426 ('91WxMwg2NHD1PwHChhbAkeCN6nQ8ikdLEx') becausewmulti_privandwmulti_pubare separate wallets with different key material (private vs. public keys). The address sequencing reflects normal index advancement (0 consumed, 1 pending, 2 available), not a mempool-triggered update. Line 456's comment should clarify this derives from the internal (change) descriptor path 84h/1h/0h/*, and the keypool assertion at line 460 confirms correct index consumption.src/wallet/rpc/backup.cpp (6)
94-101: Consistent rescan guidance for importprivkeyThe added explanation about when
rescancan be safely set tofalse, plus the updated argument description (“Scan the chain and mempool for wallet transactions.”), is clear and aligns with mempool-aware rescans. No issues.
213-214: importaddress help text matches new rescan semanticsThe extra note about using
rescanwalletwhenrescan=falseand the mempool-inclusive wording for therescanargument look consistent withimportprivkeyand the broader behavior change.Also applies to: 223-223
411-412: importpubkey rescan docs kept in syncSame pattern here: guidance for
rescan=falseand the “chain and mempool” description are consistent with the other RPCs. Good to keep these three imports aligned.Also applies to: 417-417
493-493: importwallet: explicit blockchain + mempool rescan noteThe new sentence about rescanning both blockchain and mempool and pointing users to
getwalletinfofor progress is helpful and matches the intended behavior of the post-import rescan.
1484-1485: importmulti: clearer rescan option textThe additional explanation around
rescan=falseand the updatedoptions.rescandescription (“Scan the chain and mempool… after all imports.”) are accurate and consistent with the other RPCs.Also applies to: 1529-1529
1841-1841: importdescriptors timestamp help mentions mempoolThe timestamp help now explicitly says that blocks up to 2 hours before the earliest timestamp and the mempool are scanned, which matches the mempool-rescan behavior under test.
test/functional/wallet_import_rescan.py (4)
72-104: Variant.check: dynamic confirmations and “trusted” field assertionsThe updated
checkmethod correctly derives confirmations from the node’s current height and an optionalconfirmation_height, while using0for mempool transactions when no height is passed. Matching the address entry’sconfirmationsto the tx, and asserting that confirmed txs lack a"trusted"field, is consistent with the new wallet RPC semantics and supports the mempool-rescan tests.
162-165: setup_network: add whitelist for second startupRestarting all nodes with
[email protected]ensures mempool synchronization and P2P behavior are not limited by bans/DoS scoring during the import/rescan checks. This is a targeted change and still respects the per-node prune/txindex configuration established inadd_nodes.
205-210: Loop simplification when sending second-round transactionsSwitching to
for variant in IMPORT_VARIANTS:(dropping the unused index) preserves behavior while slightly simplifying the code. No functional impact.
221-258: New mempool-rescan coverage path looks correctThe added mempool-specific test section:
- Filters out late-timestamp and pruned variants, focusing on unpruned nodes where mempool rescans are meaningful.
- Keeps the block timestamp fixed so only mempool contents change.
- Creates one unconfirmed transaction per variant, syncs mempools, and then reuses the existing
Variantimport logic andcheck()to assert:
rescan=trueimports see the mempool transaction withconfirmations==0and the expected balance and tx count.rescan=falseimports see no txs and zero balance.Node selection via
ImportNode(variant.prune, expect_rescan)matches the earlier parts of the test, and labels are distinct (“mempool label …”), so per-label assertions remain isolated even though nodes are reused. This gives good coverage of mempool-inclusive rescan behavior without altering non-mempool cases.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
ci/dash/lint-tidy.sh(1 hunks)src/.clang-tidy(1 hunks)src/bench/wallet_loading.cpp(0 hunks)src/bitcoin-cli.cpp(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/blockfilter.cpp(1 hunks)src/coins.cpp(1 hunks)src/core_read.cpp(1 hunks)src/init.cpp(1 hunks)src/logging.cpp(1 hunks)src/netbase.cpp(3 hunks)src/node/blockstorage.cpp(1 hunks)src/qt/splashscreen.cpp(1 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/mining.cpp(1 hunks)src/rpc/output_script.cpp(0 hunks)src/rpc/rawtransaction_util.cpp(1 hunks)src/rpc/util.cpp(1 hunks)src/script/descriptor.cpp(2 hunks)src/test/base58_tests.cpp(2 hunks)src/test/blockfilter_tests.cpp(1 hunks)src/test/fuzz/txorphan.cpp(1 hunks)src/test/key_io_tests.cpp(3 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(4 hunks)src/test/util_tests.cpp(1 hunks)src/test/validation_block_tests.cpp(1 hunks)src/univalue/include/univalue.h(1 hunks)src/univalue/lib/univalue.cpp(2 hunks)src/util/message.cpp(0 hunks)src/util/strencodings.cpp(0 hunks)src/util/string.cpp(1 hunks)src/util/string.h(0 hunks)src/util/threadinterrupt.h(1 hunks)src/wallet/bdb.cpp(1 hunks)src/wallet/receive.cpp(1 hunks)src/wallet/rpc/backup.cpp(8 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(3 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/rpc_wipewallettxes.py(1 hunks)test/functional/wallet_balance.py(1 hunks)test/functional/wallet_import_rescan.py(5 hunks)test/functional/wallet_importdescriptors.py(2 hunks)
💤 Files with no reviewable changes (5)
- src/util/strencodings.cpp
- src/bench/wallet_loading.cpp
- src/util/message.cpp
- src/util/string.h
- src/rpc/output_script.cpp
✅ Files skipped from review due to trivial changes (1)
- src/wallet/wallet.cpp
🚧 Files skipped from review as they are similar to previous changes (21)
- src/test/blockfilter_tests.cpp
- src/util/string.cpp
- src/test/base58_tests.cpp
- src/test/sighash_tests.cpp
- src/test/transaction_tests.cpp
- src/coins.cpp
- src/core_read.cpp
- src/wallet/scriptpubkeyman.cpp
- src/node/blockstorage.cpp
- ci/dash/lint-tidy.sh
- src/wallet/receive.cpp
- src/.clang-tidy
- src/test/util_tests.cpp
- src/univalue/include/univalue.h
- src/init.cpp
- src/test/validation_block_tests.cpp
- src/wallet/spend.cpp
- src/bitcoin-tx.cpp
- src/test/fuzz/txorphan.cpp
- src/rpc/util.cpp
- src/logging.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash 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.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash 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: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/rawtransaction_util.cppsrc/rpc/mining.cppsrc/wallet/walletdb.cppsrc/wallet/test/wallet_tests.cpptest/functional/wallet_importdescriptors.pysrc/univalue/lib/univalue.cppsrc/wallet/rpc/backup.cpptest/functional/rpc_wipewallettxes.pysrc/bitcoin-cli.cppsrc/test/script_tests.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/rpc/rawtransaction_util.cppsrc/univalue/lib/univalue.cpp
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
test/functional/wallet_balance.pytest/functional/wallet_importdescriptors.py
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/walletdb.cppsrc/wallet/test/wallet_tests.cppsrc/bitcoin-cli.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/test/wallet_tests.cpptest/functional/wallet_import_rescan.pytest/functional/rpc_wipewallettxes.py
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/netbase.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/netbase.cpp
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/wallet_import_rescan.py
🧬 Code graph analysis (4)
test/functional/wallet_balance.py (2)
src/wallet/rpc/backup.cpp (7)
descriptors(2029-2029)dumpprivkey(831-875)dumpprivkey(831-831)importaddress(207-308)importaddress(207-207)importprivkey(87-179)importprivkey(87-87)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_importdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_import_rescan.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (2)
start_nodes(660-678)sync_mempools(848-874)
test/functional/rpc_wipewallettxes.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-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: Lint / Run linters
🔇 Additional comments (36)
src/util/threadinterrupt.h (1)
9-9: LGTM! Proper include for thread safety annotations.Adding
#include <threadsafety.h>is correct since the file usesEXCLUSIVE_LOCKS_REQUIREDannotations on lines 26 and 28. This follows the "include what you use" principle and makes the header self-contained.src/test/script_tests.cpp (1)
884-884: LGTM! Avoids unnecessary copy.The change from by-value to const-reference binding eliminates an unnecessary copy of the UniValue object while preserving correctness. Since
testis only read (never modified), binding to a const reference is safe and improves efficiency.src/test/key_io_tests.cpp (1)
31-31: LGTM! Consistent copy elimination across all test cases.All three changes consistently eliminate unnecessary copies by binding to const references. Each
testvariable is only read (never modified), making the const-reference binding both safe and more efficient.Also applies to: 89-89, 129-129
src/test/miniscript_tests.cpp (1)
119-121: LGTM! Proper handling of clang-tidy false positive.The addition of the using declaration for the
_mstliteral operator (used on line 145) is correct, and the NOLINTNEXTLINE directive appropriately suppresses a known clang-tidy false positive documented in the referenced LLVM issue.test/functional/rpc_wipewallettxes.py (1)
36-37: LGTM! Test correctly verifies mempool-inclusive rescan behavior.The updated assertion correctly reflects the backported behavior change where
rescanblockchain()(when called withoutmax_height) now includes mempool transactions. After wiping the unconfirmed transaction at line 33, the rescan properly recovers it from the mempool, bringing the total from 103 to 104 transactions.src/wallet/rpc/backup.cpp (3)
94-95: LGTM: Consistent help text improvements across import RPCs.The help text updates consistently enhance user guidance by:
- Clarifying that rescanning includes both chain and mempool
- Explaining when
rescan=falseis appropriate (key never used)- Noting that
rescanwalletshould be called if the key was used butrescan=falsewas setThese documentation improvements are consistent across
importprivkey,importaddress, andimportpubkeyRPCs.Also applies to: 100-100, 213-214, 223-223, 411-412, 417-417
493-493: LGTM: Clear documentation of rescan behavior.The updated help text clearly indicates that both the blockchain and mempool will be rescanned after a successful import, improving user understanding of the operation's scope.
1484-1485: LGTM: Completed documentation updates for descriptor imports.The help text updates for
importmultiandimportdescriptorscomplete the consistent documentation improvements across all import-related RPCs. The changes appropriately indicate that rescanning includes mempool transactions in addition to blockchain transactions.Also applies to: 1529-1529, 1841-1841
src/script/descriptor.cpp (2)
550-550: LGTM!The concatenation logic is correct. The change from move semantics to direct concatenation is intentional from the upstream backport and doesn't introduce any functional issues.
574-574: LGTM!The concatenation logic is correct and consistent with the change at line 550. The modification is part of the upstream backport and maintains correct functionality.
src/qt/splashscreen.cpp (1)
58-58: LGTM – const reference binding is safe here.The change from by-value to const reference avoids a copy. Since all uses of
titleAddText(lines 112, 116, 123) occur within the constructor scope, temporary lifetime extension ensures the binding remains valid throughout its usage.src/netbase.cpp (1)
393-393: LGTM! Clean refactoring of error handling.The changes eliminate unnecessary assignments to
recvrwhen the return value is only needed for an immediate comparison. The variable is still properly used for the more complex error handling in the middle section (lines 441-479) where the specific error type matters. This simplifies the code without changing behavior.Also applies to: 416-416, 482-482
src/rpc/blockchain.cpp (1)
2530-2533: LGTM! Performance optimization through move semantics.The change from
const auto& scripttoCScript& scriptenables the move operation on line 2533, reducing overhead. The implementation is correct:
- Line 2531: Script is used (read-only) to infer the descriptor
- Line 2532: Script is copied into the
needlesset- Line 2533: Script is moved into
descriptorsmap as the keyThis optimizes from two copies to one copy + one move, while maintaining correctness since the script isn't accessed after the move.
test/functional/wallet_importdescriptors.py (1)
430-431: LGTM! Captures transaction ID for subsequent validation.The change from discarding the return value to capturing
send_txidenables the assertions below to verify mempool and unspent output state, which aligns with the mempool-rescan feature being tested.test/functional/wallet_import_rescan.py (6)
79-79: LGTM! Minor formatting adjustment.Blank line removed for consistency.
91-98: LGTM! Correctly handles mempool transactions with dynamic confirmations.The updated logic computes
confirmationsdynamically based on whetherconfirmation_heightis provided. Whenconfirmation_heightisNone(mempool transactions), confirmations correctly evaluates to 0. The assertion on line 97 ensures that confirmed transactions don't have a "trusted" field, which is the expected behavior.
102-102: LGTM! Reuses the computed confirmations value.This ensures consistency between transaction and address confirmation checks.
161-161: LGTM! Whitelist flag accelerates test execution.The whitelist flag speeds up transaction relay and mempool synchronization, which is beneficial for the mempool-rescan tests added below.
166-166: LGTM! Minor formatting adjustment.Blank line removed for consistency.
220-257: LGTM! Comprehensive mempool rescan test coverage.The new test section properly validates mempool-inclusive rescan behavior:
- Filters out
late_timestampandprunedvariants that aren't applicable to mempool testing- Creates mempool transactions with unique amounts for each import variant
- Verifies mempool size matches expectations and synchronizes across nodes
- For each variant, validates that:
- Imports with
rescan=yescorrectly pick up mempool transactions- Imports with
rescan=nocorrectly ignore mempool transactionsThe test design aligns with the PR objective of validating wallet rescanning that includes mempool when no max_height is specified.
test/functional/wallet_balance.py (1)
283-305: LGTM! Validates mempool-inclusive import behavior for legacy wallets.The new test block correctly validates that:
- importaddress flow: After importing an address to a fresh wallet, the mempool transaction appears in
watchonly.untrusted_pending(line 293)- importprivkey flow: After importing the private key, the balance moves from
watchonlytomine.untrusted_pending(lines 294-299)- Fresh wallet importprivkey flow: A new wallet with importprivkey directly sees the balance in
mine.untrusted_pending(lines 302-304)The conditional check on line 298 handles Dash-specific behavior where
watchonlyis omitted when the balance is zero, as noted in the TODO comment on line 297. This divergence from Bitcoin Core is documented and intentional.src/wallet/bdb.cpp (1)
438-448: Const-ref iteration overm_databasesis correct and avoids copiesUsing
for (const auto& it : m_databases)here is safe and slightly more efficient since you only readit.firstto buildfilenames, without changing any behavior.src/blockfilter.cpp (1)
171-176: Efficient iteration inAllBlockFilterTypesSwitching to
for (const auto& entry : g_filter_types)is a straightforward copy-elision that leaves the logic and thread-safety of thestd::call_onceinitialization unchanged.src/rpc/mining.cpp (1)
755-762: Longpolllpstrconst-ref binding is safe
lpstris a const reference to the temporary returned bylpval.get_str(), and it’s only used within the same block, so lifetime extension keeps it valid while needed; behavior matches the previous by-value code while avoiding an extra copy.src/bitcoin-cli.cpp (1)
949-955: Const-refwallet_namebinding is lifetime-safe and preserves behaviorBinding
wallet_nameasconst std::string&towallet.get_str()is safe here (the temporary’s lifetime is extended to the scope of the reference) and it’s only copied into the optionalrpcwalletparameter, so this just avoids an intermediate string copy with no semantic change.src/rpc/rawtransaction_util.cpp (1)
148-178: Const-ref bindings inParsePrevoutsare safe and reduce copiesUsing
const UniValue& prevTxs = prevTxsUnival.get_array();andconst UniValue& prevOut = p.get_obj();is safe here—each temporary’s lifetime is extended to the reference scope, and the values are only read within that scope—so behavior is unchanged while avoiding a couple of UniValue copies.src/wallet/test/wallet_tests.cpp (3)
784-785: LGTM! Helpful clarifying comment.The comment clearly explains that the two AddToWallet events correspond to block_tx and mempool_tx detected during the wallet rescan, despite notifications being blocked.
797-799: LGTM! Clear explanation of event replay.The comment correctly describes that the AddToWallet count increases to 4 because the queued notifications are now processed, causing the same transactions to trigger AddToWallet events a second time.
821-821: Correct expectation adjustment for synchronous notification scenario.The change from 4 to 2 is correct here because in this test path, transactions are created and notifications are processed synchronously (via SyncWithValidationInterfaceQueue on Line 818) rather than being queued and replayed. This results in only 2 AddToWallet events instead of 4.
src/wallet/walletdb.cpp (3)
887-891: LGTM! Efficient iteration with const reference.Using
const auto&instead ofautoavoids unnecessary copies of descriptor cache entries during iteration, improving performance without changing behavior.
894-902: LGTM! Optimized descriptor key iteration.The change to
const auto&eliminates unnecessary copies of descriptor key pairs, which improves wallet loading performance since these are non-trivial objects.
904-912: LGTM! Consistent optimization for encrypted keys.Using
const auto&here maintains consistency with the other iterator optimizations and avoids copying crypted key data during wallet loading.src/univalue/lib/univalue.cpp (4)
104-109: LGTM! Modern C++ move semantics optimization.The change from pass-by-const-reference to pass-by-value with std::move enables efficient handling of both lvalues (copy once) and rvalues (move directly), improving performance when pushing temporary UniValue objects.
118-124: LGTM! Consistent move optimization for key-value pairs.Both parameters now use pass-by-value with std::move, allowing efficient handling of temporary keys and values. This is consistent with the push_back optimization and follows modern C++ best practices.
126-135: LGTM! Proper move semantics in both code paths.The function correctly uses std::move when replacing an existing value (Line 132) and when delegating to __pushKV for new keys (Line 134), ensuring efficient handling in both scenarios.
137-144: LGTM! Efficient object merging with move semantics.Pass-by-value with std::move in the loop enables efficient transfer of key-value pairs when merging objects, particularly beneficial when the source object is a temporary.
| assert_equal(change_addr, '8y2sLiPQnB81bAeiRvwbjozJXnCCNH2nHb') | ||
| assert(send_txid in self.nodes[0].getrawmempool(True)) | ||
| assert(send_txid in (x['txid'] for x in wmulti_pub.listunspent(0))) | ||
| assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use test framework assertion helpers consistently.
Lines 458-459 use bare assert statements instead of the test framework's assert_equal or related helpers. The test framework provides better error messages and is the established pattern throughout the codebase.
Apply this diff to use framework assertions:
assert_equal(change_addr, '8y2sLiPQnB81bAeiRvwbjozJXnCCNH2nHb')
- assert(send_txid in self.nodes[0].getrawmempool(True))
- assert(send_txid in (x['txid'] for x in wmulti_pub.listunspent(0)))
+ assert send_txid in self.nodes[0].getrawmempool(True)
+ assert send_txid in (x['txid'] for x in wmulti_pub.listunspent(0))
assert_equal(wmulti_pub.getwalletinfo()['keypoolsize'], 999)Note: Alternatively, consider using assert_equal(send_txid in self.nodes[0].getrawmempool(True), True) for consistency, though the bare assert without parentheses is acceptable in Python test code.
🤖 Prompt for AI Agents
In test/functional/wallet_importdescriptors.py around lines 457 to 460, the two
bare assert statements should be converted to test framework assertions for
consistent messages and style; replace "assert(send_txid in
self.nodes[0].getrawmempool(True))" with "assert_true(send_txid in
self.nodes[0].getrawmempool(True))" (or alternatively assert_equal(..., True))
and replace "assert(send_txid in (x['txid'] for x in
wmulti_pub.listunspent(0)))" with "assert_true(send_txid in (x['txid'] for x in
wmulti_pub.listunspent(0)))" so both checks use the framework helpers.
There was a problem hiding this comment.
it's a backported code from bitcoin core; no changes will be provided in this PR until bitcoin core changes it.
There was a problem hiding this comment.
@knst, understood! I acknowledge your decision. This aligns with the project's established backport policy of maintaining fidelity to upstream Bitcoin Core code to minimize merge conflicts in future backports. The bare assert statements will remain as they are in the original Bitcoin Core implementation.
🧠 Learnings used
Learnt from: kwvg
Repo: dashpay/dash 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: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash 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.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
knst
force-pushed
the
bp-v24-p13
branch
2 times, most recently
from
ab0119c to
3c4ff33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/evo/specialtxman.cpp (1)
489-489: Restore std::move for better performance.The assignment copies the entire
newListinstead of moving it. SincenewListis a local variable that's no longer used after this line, usingstd::movewould avoid the copy overhead and improve performance.Apply this diff to restore move semantics:
- mnListRet = newList; + mnListRet = std::move(newList);Note: If this change was made intentionally to match upstream Bitcoin Core's implementation for backport consistency, please disregard this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
-
ci/dash/lint-tidy.sh(1 hunks) -
src/.clang-tidy(1 hunks) -
src/bench/load_external.cpp(1 hunks) -
src/bench/wallet_loading.cpp(0 hunks) -
src/bitcoin-cli.cpp(1 hunks) -
src/bitcoin-tx.cpp(1 hunks) -
src/blockfilter.cpp(1 hunks) -
src/bls/bls_worker.cpp(1 hunks) -
src/coins.cpp(1 hunks) -
src/core_read.cpp(1 hunks) -
src/evo/creditpool.cpp(1 hunks) -
src/evo/mnauth.cpp(1 hunks) -
src/evo/specialtxman.cpp(1 hunks) -
src/init.cpp(1 hunks) -
src/instantsend/instantsend.cpp(1 hunks) -
src/llmq/blockprocessor.cpp(1 hunks) -
src/llmq/snapshot.cpp(1 hunks) -
src/logging.cpp(1 hunks) -
src/masternode/meta.cpp(1 hunks) -
src/netbase.cpp(3 hunks) -
src/node/blockstorage.cpp(1 hunks) -
src/qt/governancelist.cpp(1 hunks) -
src/qt/splashscreen.cpp(2 hunks) -
src/qt/transactionrecord.cpp(0 hunks) -
src/rpc/blockchain.cpp(1 hunks) -
src/rpc/coinjoin.cpp(1 hunks) -
src/rpc/evo.cpp(0 hunks) -
src/rpc/masternode.cpp(0 hunks) -
src/rpc/mining.cpp(3 hunks) -
src/rpc/output_script.cpp(0 hunks) -
src/rpc/rawtransaction_util.cpp(1 hunks) -
src/rpc/util.cpp(3 hunks) -
src/script/descriptor.cpp(2 hunks) -
src/test/base58_tests.cpp(2 hunks) -
src/test/blockfilter_tests.cpp(1 hunks) -
src/test/coinstatsindex_tests.cpp(0 hunks) -
src/test/evo_deterministicmns_tests.cpp(1 hunks) -
src/test/evo_trivialvalidation.cpp(1 hunks) -
src/test/fuzz/txorphan.cpp(1 hunks) -
src/test/key_io_tests.cpp(3 hunks) -
src/test/miniscript_tests.cpp(1 hunks) -
src/test/script_tests.cpp(1 hunks) -
src/test/sighash_tests.cpp(1 hunks) -
src/test/transaction_tests.cpp(4 hunks) -
src/test/util_tests.cpp(1 hunks) -
src/test/validation_block_tests.cpp(1 hunks) -
src/univalue/include/univalue.h(1 hunks) -
src/univalue/lib/univalue.cpp(2 hunks) -
src/util/message.cpp(0 hunks) -
src/util/strencodings.cpp(0 hunks) -
src/util/string.cpp(1 hunks) -
src/util/string.h(0 hunks) -
src/util/threadinterrupt.h(1 hunks) -
src/wallet/bdb.cpp(1 hunks) -
src/wallet/receive.cpp(1 hunks) -
src/wallet/scriptpubkeyman.cpp(1 hunks) -
src/wallet/spend.cpp(1 hunks) -
src/wallet/test/bip39_tests.cpp(1 hunks) -
src/wallet/test/wallet_tests.cpp(3 hunks) -
src/wallet/walletdb.cpp(2 hunks)
💤 Files with no reviewable changes (9)
- src/rpc/output_script.cpp
- src/rpc/masternode.cpp
- src/util/strencodings.cpp
- src/rpc/evo.cpp
- src/bench/wallet_loading.cpp
- src/test/coinstatsindex_tests.cpp
- src/util/message.cpp
- src/qt/transactionrecord.cpp
- src/util/string.h
🚧 Files skipped from review as they are similar to previous changes (26)
- src/init.cpp
- src/test/util_tests.cpp
- src/node/blockstorage.cpp
- src/.clang-tidy
- src/wallet/bdb.cpp
- src/wallet/test/bip39_tests.cpp
- src/test/blockfilter_tests.cpp
- src/llmq/blockprocessor.cpp
- src/qt/governancelist.cpp
- src/blockfilter.cpp
- src/test/base58_tests.cpp
- src/test/script_tests.cpp
- src/core_read.cpp
- src/rpc/mining.cpp
- src/test/sighash_tests.cpp
- src/test/transaction_tests.cpp
- src/wallet/spend.cpp
- src/instantsend/instantsend.cpp
- src/wallet/walletdb.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/masternode/meta.cpp
- src/bitcoin-tx.cpp
- src/rpc/blockchain.cpp
- src/test/key_io_tests.cpp
- src/evo/creditpool.cpp
- src/rpc/coinjoin.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash 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.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash 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: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/bitcoin-cli.cppsrc/wallet/test/wallet_tests.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/bitcoin-cli.cppsrc/rpc/rawtransaction_util.cppsrc/wallet/test/wallet_tests.cppsrc/rpc/util.cppsrc/univalue/lib/univalue.cppsrc/univalue/include/univalue.h
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/rpc/rawtransaction_util.cppsrc/univalue/lib/univalue.cppsrc/univalue/include/univalue.h
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/test/wallet_tests.cpp
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/rpc/util.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/test/validation_block_tests.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/netbase.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/netbase.cpp
🧬 Code graph analysis (2)
src/rpc/rawtransaction_util.cpp (1)
src/rpc/request.cpp (2)
JSONRPCError(56-62)JSONRPCError(56-56)
src/univalue/include/univalue.h (1)
src/univalue/lib/univalue.cpp (12)
push_back(104-109)push_back(104-104)push_backV(111-116)push_backV(111-111)__pushKV(118-124)__pushKV(118-118)key(188-198)key(188-188)pushKV(126-135)pushKV(126-126)pushKVs(137-144)pushKVs(137-137)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/mnauth.cpp
[error] 1-999: Clang format differences found. The diff indicates formatting changes between before and after; run the clang-format-diff script or clang-format to fix formatting in this file. Command that triggered the failure: git diff -U0 origin/develop --
⏰ 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: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-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: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (30)
src/test/validation_block_tests.cpp (1)
185-185: LGTM! Optimizes loop iteration to avoid unnecessary copies.The change from
autotoconst auto&avoids copyingshared_ptrobjects in each iteration, which is beneficial in this hot loop executed across multiple threads. Since the loop body only reads fromblockand doesn't modify it, using a const reference is both correct and more efficient.src/test/fuzz/txorphan.cpp (1)
71-71: LGTM! Clean hygiene improvements.The spelling correction ("popped" instead of "poped") and the removal of the unused
#include <algorithm>header improve code quality without affecting functionality. The code usesstd::swapfrom<utility>(line 25), so the<algorithm>header is indeed unnecessary.src/util/threadinterrupt.h (1)
9-9: LGTM!The explicit include of
threadsafety.his appropriate since the class uses thread safety annotations (EXCLUSIVE_LOCKS_REQUIRED) on lines 26 and 28. This makes the dependency explicit rather than relying on transitive includes.src/bls/bls_worker.cpp (1)
18-18: LGTM! Efficient const-reference binding.The change from
auto itemtoconst auto& itemavoids unnecessary copies while preserving the validation and duplicate-detection logic.src/coins.cpp (1)
359-359: LGTM! Good optimization.Using
const auto&instead ofautoavoids unnecessary copies of the callback objects during iteration, which is especially beneficial if the callbacks arestd::functionor similar types that can be expensive to copy.src/test/miniscript_tests.cpp (1)
119-121: LGTM! User-defined literal operator properly declared.The using declaration brings the
_mstoperator into scope for cleaner syntax (used on line 145). The NOLINTNEXTLINE directive appropriately suppresses a false positive from clang-tidy regarding unused using declarations, with a reference to the relevant LLVM issue.src/netbase.cpp (1)
393-393: LGTM! Clean refactoring of error checking.The changes eliminate unnecessary intermediate variable assignments by comparing
InterruptibleRecvresults directly toIntrRecvError::OKwhen the specific error type doesn't need to be discriminated. The logic remains unchanged, and cases that do need to distinguish error types (like checking for Timeout at line 442) still properly store the result inrecvr.Also applies to: 416-416, 482-482
src/script/descriptor.cpp (2)
543-553: LGTM - Backport change is correct.The removal of
std::move(tmp)at line 550 is consistent with the upstream Bitcoin Core backport. While this changes the operation from move to copy, the functional behavior remains correct and the performance impact is negligible.
555-581: LGTM - Backport change is correct.The removal of
std::move(tmp)at line 574 is consistent with the upstream Bitcoin Core backport. Note that line 579 retains move semantics for the final concatenation, indicating this was a deliberate upstream change rather than a blanket removal of all moves. The code remains functionally correct.src/test/evo_trivialvalidation.cpp (1)
51-51: LGTM: Loop variable optimization reduces unnecessary copies.Changing the loop variable from by-value to const-reference avoids copying UniValue objects on each iteration while preserving the existing read-only access pattern.
src/wallet/scriptpubkeyman.cpp (1)
1929-1929: LGTM: Loop iteration optimization avoids unnecessary pair copies.Changing from by-value to const-reference iteration avoids copying each key-value pair from m_map_crypted_keys. The loop body only reads from key_pair, making const-reference the appropriate choice.
src/wallet/test/wallet_tests.cpp (2)
784-799: LGTM: Test expectations updated to reflect wallet rescan behavior changes.The updated assertions and clarifying comments correctly reflect the wallet's changed behavior regarding mempool transaction handling during rescans. The test now expects:
- 2 AddToWallet events after initial rescan (block_tx and mempool_tx detected)
- 4 total events after processing queued notifications (stale events counted again)
The comments help document the event counting logic at each checkpoint.
821-821: LGTM: Assertion matches immediate notification handling scenario.The expectation of 2 AddToWallet events is correct for the scenario where block and mempool transactions are created after wallet load and notifications are processed immediately.
src/qt/splashscreen.cpp (2)
119-119: LGTM!Inlining the
getBadgeColor()call is safe and reduces an intermediate variable. The temporary (if any) lives until the end of the full expression, which is afterfillRectcompletes.
58-58: No issues found. ThegetTitleAddText()method returnsconst QString&, making the const reference binding on line 58 safe. The reference points to an internal member that persists for the program's lifetime, so all subsequent uses are valid.src/llmq/snapshot.cpp (1)
212-218: LGTM: Refactoring to direct output parameter with defensive reset.The change correctly refactors from using a local variable to writing directly into
response.mnListDiffAtHMinus4C. The defensive reset on line 216 ensures the field is in a clean state ifBuildSimplifiedMNListDifffails after partial modification, preserving the previous behavior where assignment would only occur on success.Note: This is the only
BuildSimplifiedMNListDiffcall in the file that explicitly resets its output parameter on failure, which appears intentional for the optionalextraSharefield.src/univalue/lib/univalue.cpp (4)
104-109: LGTM! Correct move-semantics optimization.The pass-by-value-then-move pattern is correctly implemented. This allows the compiler to elide copies when rvalues are passed while still supporting lvalues.
Based on learnings
118-124: LGTM! Consistent move optimization for key-value insertion.Both the key and value are correctly moved into storage, following the same optimization pattern as
push_back.Based on learnings
126-135: LGTM! Efficient key-value upsert with move semantics.The implementation correctly moves the value when replacing an existing key (line 132) and moves both key and value when adding a new entry (line 134).
Based on learnings
137-144: LGTM! Bulk key-value merge with efficient move semantics.The implementation correctly moves individual keys and values from the source object. Taking
objby value allows the caller to pass either an lvalue (which will be copied then consumed) or an rvalue (which will be moved then consumed).Based on learnings
src/univalue/include/univalue.h (1)
80-87: LGTM! Public API declarations correctly updated for move semantics.The method signatures are correctly updated to match the implementations in
univalue.cpp. This API change enables the pass-by-value-then-move optimization pattern across all container modification methods.Based on learnings
src/wallet/receive.cpp (1)
431-431: LGTM: Efficient iteration with const reference.The loop now binds
_groupingby const reference instead of by value, avoiding unnecessary copies of eachstd::set<CTxDestination>during iteration. This optimization is consistent with the broader pattern in this PR.ci/dash/lint-tidy.sh (1)
47-47: LGTM: CI tooling update.Adding
src/util/string.cppto the iwyu analysis set aligns with the code changes in that file within this PR.src/util/string.cpp (1)
13-13: LGTM: Correctly removed std::move from const reference.The change removes
std::move(search)wheresearchis aconst std::string¶meter. Since you cannot meaningfully move from a const reference, this is the correct approach. Thestd::regexconstructor will copy from the const reference as intended.src/bench/load_external.cpp (1)
31-31: LGTM: Safe const-reference binding to temporary.The change binds
paramsas a const reference to the temporary returned byParams(). In C++, this extends the temporary's lifetime to match the reference's scope, andparamsis used immediately on line 32 within the same scope. This optimization avoids an unnecessary copy without introducing lifetime issues.src/logging.cpp (1)
465-465: LGTM: Safe const-reference binding avoids copy.The change binds
threadnameas a const reference to the result ofutil::ThreadGetInternalName(), avoiding an unnecessary string copy. The reference is used immediately on line 467 within the same scope, and lifetime extension ensures safety.src/rpc/util.cpp (1)
102-102: LGTM: Const-reference bindings avoid unnecessary copies.All three changes bind local variables as const references to UniValue method results (
get_str()andgetValStr()), avoiding string copies. These methods typically return references to internal storage, and all usages occur within the same function scope:
- Line 102:
strHexused on lines 103-107- Line 129:
strNumused on lines 131-132- Line 138:
strNumused on lines 140-141Also applies to: 129-129, 138-138
src/rpc/rawtransaction_util.cpp (1)
149-149: LGTM: Const-reference bindings to UniValue internal data.Both changes bind to references of UniValue's internal data structures:
- Line 149:
prevTxsreferences the array and is iterated over starting at line 150- Line 156:
prevOutreferences the object and is used throughout lines 158-227These avoid unnecessary copies while remaining safe since the references point to internal storage of the parent UniValue objects that remain alive throughout the usage scope.
Also applies to: 156-156
src/bitcoin-cli.cpp (1)
951-951: No changes required; const-reference binding is safe.UniValue::get_str() returns
const std::string&, which is a reference to internal storage rather than a temporary. The binding in the code is therefore safe—the reference directly aliases the internal string, and no temporary lifetime extension applies. The code is correct as written.src/evo/mnauth.cpp (1)
39-39: Based on my verification of the codebase, I can now address the original review comment. The first shell script confirmed thatmn_activeman.GetPubKey()at line 39 returnsCBLSPublicKey(from src/masternode/node.h:80 and src/masternode/node.cpp:292:CBLSPublicKey CActiveMasternodeManager::GetPubKey() const).The original review comment raises a concern about asymmetry between how line 39 constructs the signHash directly with
GetPubKey()versus how ProcessMessage allegedly wraps the public key. However, I need to verify whether the ProcessMessage code at lines 92-94 actually exists as described and whether this inconsistency is genuinely problematic.Given that:
- Both code paths use
CBLSPublicKeyas the public key type- The PR summary indicates tests passed
- The comment references "See comment in PushMNAUTH" suggesting intentional symmetry
- I cannot locate the exact lines 92-94 in the search results to confirm the alleged asymmetry
The original review comment appears to be speculating about a potential issue without providing definitive evidence of the actual code implementation. The concern is that different methods of creating CBLSPublicKey objects might serialize differently, but this requires:
- Verifying ProcessMessage's actual implementation at lines 92-94
- Confirming whether CBLSPublicKey(dmn->pdmnState->pubKeyOperator.Get()) and GetPubKey() actually produce different serializations
Without access to the complete ProcessMessage implementation in the search results, and given that tests reportedly passed, the review comment's severity cannot be definitively confirmed.
Verify type consistency between PushMNAUTH and ProcessMessage signHash construction.
Line 39 now uses
mn_activeman.GetPubKey()directly in the signHash tuple, while the review claims line 92 inProcessMessagewraps the public key withCBLSPublicKeyconstructor. Both should serialize identically for MNAUTH to work. Confirm that both code paths produceCBLSPublicKeyobjects that serialize the same way, especially if they call constructors differently. If tests pass, the serialization is likely compatible, but manually verify the ProcessMessage implementation at lines 92-94 to ensure symmetric signHash computation.
| auto pk = mn_activeman.GetPubKey(); | ||
| const CBLSPublicKey pubKey(pk); | ||
| const uint256 signHash{::SerializeHash(std::make_tuple(pubKey, receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion))}; | ||
| const uint256 signHash{::SerializeHash(std::make_tuple(mn_activeman.GetPubKey(), receivedMNAuthChallenge, peer.IsInboundConn(), nOurNodeVersion))}; |
There was a problem hiding this comment.
Fix clang-format violations.
The pipeline detected formatting differences in this file. Please run clang-format to fix the formatting issues.
#!/bin/bash
# Run clang-format on the file to show the expected formatting
clang-format src/evo/mnauth.cpp | diff -u src/evo/mnauth.cpp - || true🤖 Prompt for AI Agents
In src/evo/mnauth.cpp around line 39, the line creating signHash is not
formatted per project clang-format rules; run clang-format on src/evo/mnauth.cpp
(or reformat that line) so spacing and line breaks match the repository style,
then stage the updated file and re-run the pipeline. Ensure you use the
project's clang-format configuration (e.g., run clang-format src/evo/mnauth.cpp
and replace the file or run the provided formatting script) so the diff is
clean.
…-format-strings.py 5b8f248 lint: remove no-longer used exceptions from lint-format-strings.py (fanquake) Pull request description: ACKs for top commit: laanwj: ACK 5b8f248 if it passes CI hebasto: ACK 5b8f248, I've verified that all of the remained false positive cases are valid. Tree-SHA512: 25c40714d271c57fb09c963a3372b62c7b4f2e9367517cdf5c73ea82527a9c4c477f8b7857e37adc7eb9feea1f0a37435059798ddf2195dee3522bed3a6eea44
… attempt 1be7964 test, wallet: Add mempool rescan test for import RPCs (Fabian Jahr) 833ce76 rpc, wallet: Document mempool rescan after importdescriptor, importwallet (Fabian Jahr) 0e396d1 rpc, wallet: Document mempool scan after importmulti (Fabian Jahr) e6d3ef8 rpc, wallet: Document mempool scan after importpubkey (Fabian Jahr) 6d3db52 rpc, wallet: Document and test mempool scan after importprivkey (João Barbosa) 3abdbbb rpc, wallet: Document and test mempool scan after importaddress (João Barbosa) 236239b wallet: Rescan mempool for transactions as well (Fabian Jahr) Pull request description: This PR picks up the work from bitcoin#18964 and closes bitcoin#18954. It should incorporate all the unaddressed feedback from the PR: - Mempool rescan now expanded to all relevant import* RPCs - Added documentation in the help of each RPC - More tests ACKs for top commit: Sjors: re-utACK 1be7964 (only a test change) achow101: ACK 1be7964 w0xlt: reACK bitcoin@1be7964 Tree-SHA512: b62fed5f97c6c242b2af417b41c9696a1f18878483d9e1c9429791f9c05257f57a00540a9a84df23c49faf6a61c3109c22972de81540083f38b506217804fcc5
a02f3f1 tidy: use misc-unused-using-decls (fanquake) d6787bc refactor: remove unused using directives (fanquake) 3617634 validation: remove unused using directives (eugene) Pull request description: Adds https://clang.llvm.org/extra/clang-tidy/checks/misc/unused-using-decls.html to our clang-tidy. PR'd after the discussion in bitcoin#25433 (which it includes). ACKs for top commit: jamesob: Github ACK bitcoin@a02f3f1 Tree-SHA512: 2bb937c1cc90006e69054458d845fb54f287567f4309c773a3fc859f260558c32ff51fc1c2ce9b43207426f3547e7ce226c87186103d741d5efcca19cd355253
f345dc3 tidy: enable bugprone-use-after-move (fanquake) 94f2235 test: work around bugprone-use-after-move warnings in util tests (fanquake) Pull request description: Would have caught bitcoin#25640. Currently `// NOLINT`s around: ```bash test/util_tests.cpp:2513:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors] BOOST_CHECK(v2[0].origin == &t2); ^ test/util_tests.cpp:2511:15: note: move occurred here auto v2 = Vector(std::move(t2)); ^ test/util_tests.cpp:2519:34: error: 't2' used after it was moved [bugprone-use-after-move,-warnings-as-errors] BOOST_CHECK(v3[1].origin == &t2); ^ test/util_tests.cpp:2516:15: note: move occurred here auto v3 = Vector(t1, std::move(t2)); ^ test/util_tests.cpp:2527:34: error: 't3' used after it was moved [bugprone-use-after-move,-warnings-as-errors] BOOST_CHECK(v4[2].origin == &t3); ^ test/util_tests.cpp:2523:15: note: move occurred here auto v4 = Vector(std::move(v3[0]), v3[1], std::move(t3)); ``` See: https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-use-after-move.html ACKs for top commit: ryanofsky: Code review ACK f345dc3. Only change since last review is switching to NOLINT directives Tree-SHA512: afadecbaf1069653f4be5d6e66a5800ffd975c0b1a960057abc6367b616c181cd518897a874a8f3fd5e5e1f45fcc165f7a9a3171136cd4deee641214c4b765b8
… unnecessarily copying objects and enable two clang-tidy checks
BACKPORT NOTE:
Somehow there's multiple warnings that are for backported code, mostly related to mempool, such as:
rpc/mempool.cpp:549:33: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
549 | for (CTxMemPool::txiter descendantIt : setDescendants) {
| ^
| const &
I can't find a way to fix it without big diversity of codebase, so, make as partial temporary
ae7ae36 tidy: Enable two clang-tidy checks (Aurèle Oulès)
081b0e5 refactor: Make const refs vars where applicable (Aurèle Oulès)
Pull request description:
I added const references to some variables to avoid unnecessarily copying objects.
Also added two clang-tidy checks : [performance-for-range-copy](https://releases.llvm.org/11.1.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-for-range-copy.html) and [performance-unnecessary-copy-initialization](https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-unnecessary-copy-initialization.html).
ACKs for top commit:
vasild:
ACK ae7ae36
MarcoFalke:
review ACK ae7ae36
Tree-SHA512: f6ac6b0cd0eee1e0c34d2f186484bc0f7ec6071451cccb33fa88a67d93d92b304e2fac378b88f087e94657745bca4e966dbc443759587400eb01b1f3061fde8c
850b085 fix comment spellings from the codespell lint (Greg Weber) Pull request description: test/lint/all-lint.py includes the codespell lint ACKs for top commit: aureleoules: ACK 850b085. Tree-SHA512: bf63690da2652886e705d6594903bab67ff0f35a0e5a5505f063827f5148ebce47681e541cbe0e52396baf1addb25d9fe50e5faa9176456f579a7cd2f1321c44
BACKPORT NOTE: Some missing changes for TRDescriptor and for mempool_args But they will be caugth by linter ---- fa87534 Fix iwyu (MacroFake) faad673 Fix issues when calling std::move(const&) (MacroFake) Pull request description: Passing a symbol to `std::move` that is marked `const` is a no-op, which can be fixed in two ways: * Remove the `const`, or * Remove the `std::move` ACKs for top commit: ryanofsky: Code review ACK fa87534. Looks good. Good for univalue to support c++11 move optimizations Tree-SHA512: 3dc5cad55b93cfa311abedfb811f35fc1b7f30a1c68561f15942438916c7de25e179c364be11881e01f844f9c2ccd71a3be55967ad5abd2f35b10bb7a882edea
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/evo/mnauth.cpp (1)
39-39: Fix clang-format violations.The formatting issue on this line was previously flagged. Please run
clang-formaton this file to fix the formatting differences detected by the pipeline.test/functional/wallet_balance.py (1)
283-305: Legacy mempool-import balance test logic looks correctThe new legacy-only block properly exercises
importaddressandimportprivkeyon fresh wallets with an unconfirmed tx, and themine/watchonlyuntrusted_pendingexpectations plus the conditionalwatchonlykey check match the stated Dash-vs-Bitcoin behavior. I don’t see correctness issues here.Based on learnings
🧹 Nitpick comments (1)
test/functional/rpc_wipewallettxes.py (1)
36-37: Test correctly validates mempool scanning behavior.The test properly validates that
rescanblockchain()now scans the mempool when called without parameters. The unconfirmed transaction from line 31 remains in the mempool afterwipewallettxes(True)removes it from the wallet, andrescanblockchain()successfully recovers it, restoring the transaction count to 104.Optional: Consider verifying the specific transaction for robustness.
The test currently only checks the transaction count. For additional robustness, you could verify that the specific transaction (
txid) is recovered:self.nodes[0].rescanblockchain() assert_equal(self.nodes[0].getwalletinfo()["txcount"], 104) # Verify the specific unconfirmed transaction was recovered from mempool self.nodes[0].gettransaction(txid)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (67)
ci/dash/lint-tidy.sh(1 hunks)src/.clang-tidy(1 hunks)src/bench/load_external.cpp(1 hunks)src/bench/wallet_loading.cpp(0 hunks)src/bitcoin-cli.cpp(1 hunks)src/bitcoin-tx.cpp(1 hunks)src/blockfilter.cpp(1 hunks)src/bls/bls_worker.cpp(1 hunks)src/coins.cpp(1 hunks)src/core_read.cpp(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/mnauth.cpp(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/init.cpp(1 hunks)src/instantsend/instantsend.cpp(1 hunks)src/llmq/blockprocessor.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/logging.cpp(1 hunks)src/masternode/meta.cpp(1 hunks)src/netbase.cpp(3 hunks)src/node/blockstorage.cpp(1 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/splashscreen.cpp(2 hunks)src/qt/transactionrecord.cpp(0 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/coinjoin.cpp(1 hunks)src/rpc/evo.cpp(0 hunks)src/rpc/masternode.cpp(0 hunks)src/rpc/mining.cpp(3 hunks)src/rpc/output_script.cpp(0 hunks)src/rpc/rawtransaction_util.cpp(1 hunks)src/rpc/util.cpp(3 hunks)src/script/descriptor.cpp(2 hunks)src/test/base58_tests.cpp(2 hunks)src/test/blockfilter_tests.cpp(1 hunks)src/test/coinstatsindex_tests.cpp(0 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/evo_trivialvalidation.cpp(1 hunks)src/test/fuzz/txorphan.cpp(1 hunks)src/test/key_io_tests.cpp(3 hunks)src/test/miniscript_tests.cpp(1 hunks)src/test/script_tests.cpp(1 hunks)src/test/sighash_tests.cpp(1 hunks)src/test/transaction_tests.cpp(4 hunks)src/test/util_tests.cpp(1 hunks)src/test/validation_block_tests.cpp(1 hunks)src/univalue/include/univalue.h(1 hunks)src/univalue/lib/univalue.cpp(2 hunks)src/util/message.cpp(0 hunks)src/util/strencodings.cpp(0 hunks)src/util/string.cpp(1 hunks)src/util/string.h(0 hunks)src/util/threadinterrupt.h(1 hunks)src/wallet/bdb.cpp(1 hunks)src/wallet/receive.cpp(1 hunks)src/wallet/rpc/backup.cpp(8 hunks)src/wallet/scriptpubkeyman.cpp(1 hunks)src/wallet/spend.cpp(1 hunks)src/wallet/test/bip39_tests.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(3 hunks)src/wallet/wallet.cpp(2 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/rpc_wipewallettxes.py(1 hunks)test/functional/wallet_balance.py(1 hunks)test/functional/wallet_import_rescan.py(5 hunks)test/functional/wallet_importdescriptors.py(2 hunks)test/lint/run-lint-format-strings.py(0 hunks)
💤 Files with no reviewable changes (10)
- src/qt/transactionrecord.cpp
- src/util/string.h
- src/test/coinstatsindex_tests.cpp
- src/rpc/evo.cpp
- src/util/strencodings.cpp
- src/rpc/output_script.cpp
- src/rpc/masternode.cpp
- src/util/message.cpp
- src/bench/wallet_loading.cpp
- test/lint/run-lint-format-strings.py
✅ Files skipped from review due to trivial changes (2)
- src/test/fuzz/txorphan.cpp
- src/test/miniscript_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (31)
- src/rpc/coinjoin.cpp
- src/qt/governancelist.cpp
- src/util/threadinterrupt.h
- src/evo/specialtxman.cpp
- src/rpc/rawtransaction_util.cpp
- src/core_read.cpp
- src/script/descriptor.cpp
- src/wallet/test/wallet_tests.cpp
- src/test/blockfilter_tests.cpp
- src/node/blockstorage.cpp
- src/instantsend/instantsend.cpp
- src/evo/creditpool.cpp
- src/coins.cpp
- src/blockfilter.cpp
- ci/dash/lint-tidy.sh
- src/test/util_tests.cpp
- src/masternode/meta.cpp
- src/llmq/blockprocessor.cpp
- src/test/key_io_tests.cpp
- src/wallet/receive.cpp
- src/test/base58_tests.cpp
- src/test/evo_trivialvalidation.cpp
- src/test/sighash_tests.cpp
- src/wallet/bdb.cpp
- src/rpc/blockchain.cpp
- src/init.cpp
- src/wallet/scriptpubkeyman.cpp
- src/logging.cpp
- src/util/string.cpp
- src/bitcoin-cli.cpp
- src/test/script_tests.cpp
🧰 Additional context used
🧠 Learnings (27)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Learnt from: kwvg
Repo: dashpay/dash 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.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Learnt from: kwvg
Repo: dashpay/dash 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: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/test/bip39_tests.cppsrc/rpc/util.cppsrc/test/transaction_tests.cpptest/functional/rpc_wipewallettxes.pysrc/wallet/walletdb.cppsrc/univalue/lib/univalue.cppsrc/wallet/wallet.cpptest/functional/wallet_importdescriptors.pysrc/wallet/rpc/backup.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/test/bip39_tests.cpptest/functional/rpc_wipewallettxes.pytest/functional/wallet_import_rescan.pytest/functional/wallet_importdescriptors.py
📚 Learning: 2025-11-14T23:17:08.495Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6976
File: src/stacktraces.cpp:475-482
Timestamp: 2025-11-14T23:17:08.495Z
Learning: In C++ code reviews, always check for opportunities to use standard algorithms (std::any_of, std::all_of, std::none_of, std::find_if, std::transform, std::for_each, etc.) instead of manual for-loops, especially when the loop body performs simple checks, transformations, or accumulations.
Applied to files:
src/test/validation_block_tests.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/test/validation_block_tests.cpptest/functional/wallet_importdescriptors.py
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/spend.cppsrc/wallet/wallet.cpptest/functional/wallet_importdescriptors.py
📚 Learning: 2025-10-03T11:29:36.358Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/governance/signing.cpp:84-85
Timestamp: 2025-10-03T11:29:36.358Z
Learning: After bitcoin#25153 (included in dash#6775), UniValue uses the templated getInt<T>() API instead of the legacy get_int() and get_int64() methods. For example, use jproposal["start_epoch"].getInt<int64_t>() to retrieve int64_t values from UniValue objects.
Applied to files:
src/rpc/util.cpp
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/evo/mnauth.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: The univalue library (in src/univalue/) is no longer a vendored external dependency but is now part of the Bitcoin Core codebase and can be modified as needed during backports.
Applied to files:
src/univalue/include/univalue.hsrc/univalue/lib/univalue.cpptest/functional/wallet_importdescriptors.py
📚 Learning: 2025-06-20T23:32:16.225Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
test/functional/wallet_balance.pytest/functional/wallet_importdescriptors.py
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/rpc/mining.cpp
📚 Learning: 2025-08-10T13:52:46.289Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6666
File: test/functional/rpc_netinfo.py:73-81
Timestamp: 2025-08-10T13:52:46.289Z
Learning: In the Bitcoin/Dash test framework, `self.node.extra_args` contains the original startup arguments for a TestNode and is not modified by `test.restart_node()`. Each restart with new `extra_args` doesn't update the stored `self.node.extra_args`, so copying from it always provides a clean slate without accumulated modifications from previous restarts.
Applied to files:
test/functional/wallet_import_rescan.py
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/wallet.cpp
📚 Learning: 2025-01-14T08:37:16.955Z
Learnt from: kwvg
Repo: dashpay/dash 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.
Applied to files:
src/wallet/wallet.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/netbase.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/netbase.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/netbase.cpp
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/test/evo_deterministicmns_tests.cpp
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash 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.
Applied to files:
src/test/evo_deterministicmns_tests.cpp
📚 Learning: 2025-11-13T20:02:55.480Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/test/evo_deterministicmns_tests.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
test/functional/wallet_importdescriptors.py
📚 Learning: 2025-07-17T15:48:29.418Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Applied to files:
test/functional/wallet_importdescriptors.py
🧬 Code graph analysis (6)
src/univalue/include/univalue.h (1)
src/univalue/lib/univalue.cpp (12)
push_back(104-109)push_back(104-104)push_backV(111-116)push_backV(111-111)__pushKV(118-124)__pushKV(118-118)key(188-198)key(188-188)pushKV(126-135)pushKV(126-126)pushKVs(137-144)pushKVs(137-137)
test/functional/rpc_wipewallettxes.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_balance.py (2)
src/wallet/rpc/backup.cpp (7)
descriptors(2029-2029)dumpprivkey(831-875)dumpprivkey(831-831)importaddress(207-308)importaddress(207-207)importprivkey(87-179)importprivkey(87-87)test/functional/test_framework/util.py (1)
assert_equal(69-74)
test/functional/wallet_import_rescan.py (2)
test/functional/test_framework/util.py (1)
assert_equal(69-74)test/functional/test_framework/test_framework.py (2)
start_nodes(646-664)sync_mempools(834-860)
src/wallet/wallet.cpp (1)
src/wallet/wallet.h (1)
cs_wallet(592-592)
test/functional/wallet_importdescriptors.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🔇 Additional comments (40)
src/bitcoin-tx.cpp (1)
558-558: LGTM! Valid optimization to avoid unnecessary copy.Changing from a copy to a const reference is a good optimization that avoids copying the UniValue object on each loop iteration. The reference is used read-only throughout the loop body, making this change safe and beneficial for performance.
src/bls/bls_worker.cpp (1)
18-18: LGTM! Efficient iteration with const reference.The change from by-value to by-const-reference iteration avoids unnecessary copying while maintaining the same semantics. Since
itemis only used for read-only operations (IsValid()andGetHash()), the const reference is both correct and more efficient.src/test/evo_deterministicmns_tests.cpp (1)
889-892: LGTM: Tests assignment operator cache-sharing behavior.The refactoring from copy-construction to default-construction followed by assignment intentionally exercises a different code path (copy assignment operator) to verify that the SML cache is properly shared. This complements the copy-constructor test at line 886-887.
src/wallet/test/bip39_tests.cpp (1)
28-28: LGTM! Const reference binding reduces copies.The change from value binding to const reference binding eliminates unnecessary copying of the
UniValuetest object while ensuring immutability in the test loop.src/test/transaction_tests.cpp (4)
156-156: LGTM! Const reference binding improves efficiency.Binding
testby const reference instead of copying reduces overhead in the test loop.
175-175: LGTM! Const reference binding prevents unnecessary copy.Binding
vinputby const reference eliminates copying of the array returned byget_array().
240-240: LGTM! Const reference binding reduces copies.Binding
testby const reference instead of copying improves efficiency in the tx_invalid test loop.
259-259: LGTM! Const reference binding eliminates copy overhead.Binding
vinputby const reference prevents copying of the array object in the tx_invalid test path.src/test/validation_block_tests.cpp (1)
185-185: LGTM! Const reference binding avoids shared_ptr copies.The change from
auto blocktoconst auto& blockeliminates unnecessary copying ofshared_ptrobjects (and associated reference count manipulation) while iterating over the blocks vector.src/wallet/rpc/backup.cpp (6)
94-95: LGTM! Documentation accurately reflects mempool scanning.The help text updates correctly clarify that the rescan parameter affects both blockchain and mempool scanning, and provide appropriate guidance for users about when to use rescanwallet.
Also applies to: 100-100
213-214: LGTM! Consistent documentation improvements.The help text updates match the pattern from importprivkey and accurately describe the rescan behavior including mempool scanning.
Also applies to: 223-223
411-412: LGTM! Documentation updates are consistent.The help text correctly describes the rescan parameter behavior including mempool scanning, consistent with the other import RPC methods.
Also applies to: 417-417
493-493: LGTM! Clear documentation of rescan behavior.The note accurately reflects that both blockchain and mempool are rescanned after a successful import.
1484-1485: LGTM! Documentation improvements are consistent.The help text updates accurately describe that rescan operations include both blockchain and mempool scanning, consistent with the other import RPC methods.
Also applies to: 1529-1529
1841-1841: LGTM! Documentation complete and accurate.The timestamp description correctly indicates that mempool scanning is included along with blockchain scanning, completing the consistent documentation improvements across all import RPC methods.
src/qt/splashscreen.cpp (2)
58-58: LGTM: Safe optimization to const reference.The const reference binding avoids copying the QString while remaining safe—the temporary's lifetime extends through the constructor scope, covering all uses of
titleAddText(lines 112, 116, 122).
119-119: LGTM: Eliminates unnecessary intermediate variable.Directly passing the return value of
getBadgeColor()tofillRectis a straightforward optimization that avoids an intermediate temporary.src/.clang-tidy (3)
1-14: LGTM! Expanded clang-tidy checks improve code quality.The addition of
bugprone-use-after-move,misc-unused-using-decls, and the three performance-related checks strengthens static analysis coverage.
26-28: LGTM! CheckOptions configuration is appropriate.Disabling
CheckTriviallyCopyableMovefor theperformance-move-const-argcheck reduces false positives when moving trivially copyable types.
15-25: Verify that performance-for-range-copy exclusion from WarningsAsErrors matches upstream Bitcoin Core.This is a backport PR, and backported changes should match the upstream configuration. The
performance-for-range-copycheck is enabled in the Checks section but not in WarningsAsErrors, while other performance checks are elevated. Confirm this configuration matches the current upstream Bitcoin Core .clang-tidy file to ensure the backport is accurate.src/llmq/snapshot.cpp (1)
215-216: LGTM! Clean refactoring to eliminate intermediate variable.The change correctly refactors the code to write directly to
response.mnListDiffAtHMinus4Cinstead of using an intermediate local variable. The defensive reset on failure (line 216) ensures a clean state ifBuildSimplifiedMNListDiffpartially modified the output before failing.src/wallet/spend.cpp (1)
316-322: LGTM! Move optimization is safe here.The change from
const COutput&toCOutput&enables moving coin objects into the result map. This is safe becauseAvailableCoinsListUnspent(wallet).all()returns a temporary vector, so moving from its elements avoids unnecessary copies without affecting other code.src/bench/load_external.cpp (1)
31-31: LGTM! Proper const-reference binding.The change binds the temporary returned by
Params()to a const reference, extending its lifetime to the end of the scope and avoiding an unnecessary copy.src/netbase.cpp (3)
393-396: LGTM! Simplified error checking.The direct comparison eliminates the unnecessary intermediate variable assignment when the result is only used once.
416-418: LGTM! Consistent with earlier simplification.
482-484: LGTM! Consistent pattern applied.src/rpc/mining.cpp (3)
134-134: LGTM! Const-reference binding avoids copy.The const reference binding extends the lifetime of the
Params()return value through the function scope, avoiding an unnecessary copy of theCChainParamsobject.
359-359: LGTM! Consistent optimization pattern.
758-758: LGTM! Avoids string copy.Binding the result of
lpval.get_str()to a const reference avoids copying the string.src/rpc/util.cpp (3)
102-107: LGTM! Const-reference binding reduces string copies.The const reference binding to
v.get_str()avoids copying the string. The lifetime is safe becausev(the UniValue parameter) outlives thestrHexreference within the function scope.
129-133: LGTM! Consistent optimization pattern.
138-142: LGTM! Consistent optimization pattern.src/univalue/include/univalue.h (1)
80-87: LGTM! Public API change enables move semantics.These signature changes from const-reference parameters to by-value parameters are intentional to enable move semantics. The implementation (visible in src/univalue/lib/univalue.cpp) uses
std::moveto efficiently transfer ownership. This is part of the upstream backport and aligns with the broader pattern of move optimizations in this PR.Based on learnings
src/wallet/wallet.cpp (1)
1685-1690: Mempool scan after full rescan is well‑gated and consistentThe updated comment and the
if (!max_height) { ... requestMempoolTransactions(*this); }block correctly extend full rescans to also cover the current mempool, and the cs_wallet locking pattern matches existing usage inpostInitProcess. No correctness or locking issues spotted.Also applies to: 1810-1813
src/univalue/lib/univalue.cpp (1)
104-109: Move‑aware UniValue mutators look correctSwitching these methods to take arguments by value and moving into internal storage avoids extra copies while preserving external semantics.
pushKVonly moveskeyafterfindKeyis done with it, andpushKVsiterates over a by‑value copy, avoiding aliasing issues. No issues found.Also applies to: 118-135, 137-144
src/wallet/walletdb.cpp (1)
887-912: Descriptor cache/key + mnemonic loading logic is coherentThe const‑ref loops over descriptor caches and keys are fine, and the new
AddKey/AddCryptedKeycalls correctly thread optional mnemonic data frommnemonics/crypted_mnemonics, falling back to empty values for legacy entries. Assumptions aboutGetScriptPubKeyMan(desc_id)being non‑null match the surrounding descriptor loading logic. No issues identified.test/functional/wallet_importdescriptors.py (1)
430-459: Multisig mempool visibility checks are well-wiredCapturing
send_txidand asserting its presence in node0’s mempool and inwmulti_pub.listunspent(0)correctly verifies that the descriptor-based watch-only wallet sees the unconfirmed multisig spend. This matches the intended mempool-rescan behavior and stays aligned with the upstream backport.Based on learnings
test/functional/wallet_import_rescan.py (3)
71-103: Confirmations and address-level checks are computed consistentlyUsing
confirmation_heightto deriveconfirmationsand applying the same value to both thelisttransactionsentry and thelistreceivedbyaddressentry is consistent, and treating a missing height as “mempool (0 conf)” matches the test’s intent. The optional"trusted"key assertion for confirmed txs is also safe in this context.
147-163: Whitelist restart in setup_network is appropriate for mempool-focused testsRestarting all nodes with
[email protected]after seeding coinbase keys is a reasonable way to ensure fast, reliable mempool propagation across the IMPORT_NODES matrix without altering the prune/txindex settings established earlier.
203-257: Mempool rescan variants are correctly isolated and verifiedThe new mempool path (reusing non-pruned, non-
late_timestampvariants, creating mempool-only txs, asserting mempool size, and then importing on the appropriate nodes based onrescanflag) cleanly tests that a rescan covers mempool entries while non-rescanning imports leave balances and tx counts at zero. State is reinitialized per variant, so earlier on-chain checks remain unaffected.
|
This pull request has conflicts, please rebase. |
4 participants
What was done?
Some more regular backports from Bitcoin Core v24
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: