backport: auto 20251204 #7048
backport: auto 20251204 #7048
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request refactors coin collection APIs to bucket coins by OutputType and renames several accessors to PascalCase (e.g., size()/all()/clear() → Size()/All()/Clear()). CoinsResult gains new methods (Add, Erase, Shuffle) and AvailableCoins/AvailableCoinsListUnspent call sites are updated accordingly. Many call sites across wallet, RPC, bench, and tests change .all()/.size()/.clear() to .All()/.Size()/.Clear() and replace direct legacy/other fields with coins[OutputType::…]. Other edits include adding OutputType::UNKNOWN and making OUTPUT_TYPES a static constexpr, minor documentation and RPC help text updates, removal of a Boost Valgrind suppression, Qt test initialization adjustments, and a functional test change to assert an RPC error for negative feerates. Sequence Diagram(Not applicable) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Key areas requiring careful attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
PastaPastaPasta
force-pushed
the
backport/auto-20251204
branch
2 times, most recently
from
40cda53 to
4bd20fc
Compare
![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)
test/functional/mempool_accept.py (1)
80-85: Optional: Consider callingtestmempoolacceptdirectly for consistency.The test correctly verifies that negative
maxfeeratevalues raise an RPC error. However, wrappingcheck_mempool_resulthere is inconsistent with the existing pattern in this file (lines 66-69), whereassert_raises_rpc_errorcallsnode.testmempoolacceptdirectly. Theresult_expected=Noneparameter is also never evaluated since the exception is raised before any result comparison.For consistency with existing patterns, consider:
- # Check negative feerate - assert_raises_rpc_error(-3, "Amount out of range", lambda: self.check_mempool_result( - result_expected=None, - rawtxs=[raw_tx_in_block], - maxfeerate=-0.01, - )) + # Check negative feerate + assert_raises_rpc_error(-3, "Amount out of range", lambda: node.testmempoolaccept( + rawtxs=[raw_tx_in_block], + maxfeerate=-0.01, + ))Note: As this is a backport PR, the current implementation is functionally correct and may match the upstream version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ffc87a94357a9c1082583d1729211b1df6cc16e8 and 4bd20fcb882953880c3b653bf7baeae7b6a7b0ba.
📒 Files selected for processing (19)
contrib/valgrind.supp(0 hunks)doc/zmq.md(1 hunks)src/bench/coin_selection.cpp(1 hunks)src/coinjoin/client.cpp(1 hunks)src/outputtype.cpp(1 hunks)src/outputtype.h(1 hunks)src/qt/test/test_main.cpp(3 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/test/versionbits_tests.cpp(1 hunks)src/wallet/coinjoin.cpp(3 hunks)src/wallet/rpc/backup.cpp(4 hunks)src/wallet/rpc/coins.cpp(1 hunks)src/wallet/spend.cpp(8 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/availablecoins_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(18 hunks)src/wallet/test/wallet_tests.cpp(4 hunks)test/functional/mempool_accept.py(1 hunks)
💤 Files with no reviewable changes (1)
- contrib/valgrind.supp
🚧 Files skipped from review as they are similar to previous changes (8)
- src/rpc/masternode.cpp
- doc/zmq.md
- src/qt/test/test_main.cpp
- src/wallet/rpc/backup.cpp
- src/wallet/rpc/coins.cpp
- src/wallet/coinjoin.cpp
- src/rpc/evo.cpp
- src/coinjoin/client.cpp
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/bench/coin_selection.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/spend.hsrc/wallet/test/coinselector_tests.cppsrc/outputtype.cppsrc/outputtype.hsrc/wallet/spend.cppsrc/test/versionbits_tests.cppsrc/wallet/test/availablecoins_tests.cpp
src/bench/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Performance benchmarks in src/bench/ must use the nanobench library
Files:
src/bench/coin_selection.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/wallet/test/wallet_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cppsrc/wallet/test/availablecoins_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/test/wallet_tests.cppsrc/wallet/spend.hsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.cppsrc/wallet/test/availablecoins_tests.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests in test/functional/ must be written in Python (minimum version specified in .python-version) and depend on dashd and dash-node
Files:
test/functional/mempool_accept.py
🧠 Learnings (10)
📓 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
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.
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/bench/coin_selection.cppsrc/wallet/test/wallet_tests.cppsrc/wallet/spend.hsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.cppsrc/wallet/test/availablecoins_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/wallet/test/wallet_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.cppsrc/wallet/test/availablecoins_tests.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/wallet/test/wallet_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cppsrc/wallet/test/availablecoins_tests.cpp
📚 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/test/wallet_tests.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/wallet/test/wallet_tests.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cppsrc/wallet/test/availablecoins_tests.cpp
📚 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.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/test/versionbits_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:
test/functional/mempool_accept.py
🧬 Code graph analysis (2)
src/wallet/test/wallet_tests.cpp (1)
src/wallet/spend.cpp (2)
AvailableCoinsListUnspent(300-303)AvailableCoinsListUnspent(300-300)
test/functional/mempool_accept.py (1)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)
⏰ 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). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (11)
src/test/versionbits_tests.cpp (1)
187-187: Test fixture change requires verification that BasicTestingSetup provides all necessary dependencies.The change from
TestingSetuptoBasicTestingSetupappears reasonable since versionbits tests create their own fake blockchain viaVersionBitsTesterand shouldn't require the full validation environment. However, verify that all test assertions pass with the lighter fixture to confirm no hidden dependencies exist. Additionally, clarify whether this fixture downgrade is within the scope of this PR or if it's an ancillary improvement.src/outputtype.h (1)
21-24: LGTM: OUTPUT_TYPES refactored to static constexpr.The change from external linkage to a static constexpr array is appropriate for this backport. The array now contains both
OutputType::LEGACYandOutputType::UNKNOWN, aligning with the expanded OutputType usage throughout the codebase.src/outputtype.cpp (1)
26-28: LGTM: ParseOutputType expanded to handle UNKNOWN.The addition of
OUTPUT_TYPE_STRING_UNKNOWNparsing is consistent with the existingLEGACYhandling and aligns with the broader refactor to support multiple output types.src/wallet/test/wallet_tests.cpp (1)
610-610: LGTM: Updated to use new CoinsResult API.All accessor calls correctly updated from lowercase to PascalCase naming convention:
.size()→.Size()and.all()→.All(). Changes are consistent with the CoinsResult refactor across the codebase.Also applies to: 620-620, 1308-1308, 1364-1364
src/bench/coin_selection.cpp (1)
60-60: LGTM: Coin access updated for per-type map structure.The change from
available_coins.legacytoavailable_coins.coins[OutputType::LEGACY]correctly reflects the new map-based storage indexed by OutputType. Benchmark logic remains unchanged.src/wallet/spend.h (1)
36-51: LGTM: CoinsResult refactored to per-type map structure.The refactor consolidates coin storage into a
std::map<OutputType, std::vector<COutput>>, replacing separate per-type vectors. Key improvements:
- Method renames to PascalCase (
All(),Size(),Clear()) improve consistency- New mutating methods (
Erase(),Shuffle(),Add()) provide better encapsulation- Per-type organization enables cleaner separation of LEGACY and UNKNOWN outputs
This is a foundational change that propagates throughout wallet code and tests.
src/wallet/test/availablecoins_tests.cpp (1)
65-66: LGTM: Tests updated for per-type coin access.Test assertions correctly updated to use
Size()and access coins via OutputType keys (coins[OutputType::UNKNOWN],coins[OutputType::LEGACY]). Test logic remains unchanged.Also applies to: 80-80
src/wallet/test/coinselector_tests.cpp (1)
88-88: LGTM: Comprehensive test suite updates for new CoinsResult API.All coin selector tests correctly updated to use the new interface:
- Accessor renames:
.all()→.All(),.clear()→.Clear()- Per-type access:
coins[OutputType::LEGACY].emplace_back(...)- Helper function updates:
GroupCoins(available_coins.All())Test logic and assertions preserved throughout. The extensive updates demonstrate thorough coverage of the API surface changes.
Also applies to: 309-320, 336-336, 363-363, 379-379, 393-393, 415-663, 940-940
src/wallet/spend.cpp (3)
80-126: LGTM: CoinsResult method implementations are straightforward.The new methods are implemented correctly:
Size(): Iterates over all OutputType buckets to compute totalAll(): Concatenates all per-type vectors with proper reservationClear(): Clears the coins mapErase(): Removes a preset coin from any bucketShuffle(): Shuffles each per-type vector independentlyAdd(): Appends to the appropriate OutputType bucketImplementations are efficient and consistent with the per-type map structure.
127-136: LGTM: GetOutputType mapping aligns with Dash output types.The helper correctly maps:
SCRIPTHASH(P2SH) →OutputType::LEGACYPUBKEYHASH(P2PKH) →OutputType::LEGACY- All others →
OutputType::UNKNOWNThis is appropriate for Dash, which primarily uses legacy output types. The mapping ensures proper bucketing for coin selection.
260-279: LGTM: Coin population and selection logic updated consistently.All coin handling paths correctly updated to use the new API:
- Coin population uses
GetOutputType()+Add()to bucket by type (lines 260-279)- Size checks use
Size()instead ofsize()(line 291)- Coin listing uses
All()for aggregation (line 347)- Selection attempts iterate over per-type buckets (lines 477-496)
- Preset coin removal uses
Erase()(line 637)- Shuffling uses new
Shuffle()method (line 653)Logic flow preserved, only access patterns changed.
Also applies to: 291-291, 347-347, 477-496, 613-613, 637-637, 649-653
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 4bd20fcb882953880c3b653bf7baeae7b6a7b0ba
Ideally, rebase before merge to make sure CI is happy
Co-authored-by: glozow <gloriajzhao@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>
…nwallet/rescanblockchain/) Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Claude Code <claude@anthropic.com>
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
PastaPastaPasta
force-pushed
the
backport/auto-20251204
branch
from
4bd20fc to
e12871f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4bd20fcb882953880c3b653bf7baeae7b6a7b0ba and e12871f.
📒 Files selected for processing (19)
contrib/valgrind.supp(0 hunks)doc/zmq.md(1 hunks)src/bench/coin_selection.cpp(1 hunks)src/coinjoin/client.cpp(1 hunks)src/outputtype.cpp(1 hunks)src/outputtype.h(1 hunks)src/qt/test/test_main.cpp(3 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/test/versionbits_tests.cpp(1 hunks)src/wallet/coinjoin.cpp(3 hunks)src/wallet/rpc/backup.cpp(4 hunks)src/wallet/rpc/coins.cpp(1 hunks)src/wallet/spend.cpp(8 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/availablecoins_tests.cpp(2 hunks)src/wallet/test/coinselector_tests.cpp(18 hunks)src/wallet/test/wallet_tests.cpp(4 hunks)test/functional/mempool_accept.py(1 hunks)
💤 Files with no reviewable changes (1)
- contrib/valgrind.supp
✅ Files skipped from review due to trivial changes (1)
- src/wallet/rpc/backup.cpp
🚧 Files skipped from review as they are similar to previous changes (10)
- src/bench/coin_selection.cpp
- src/qt/test/test_main.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/rpc/coins.cpp
- src/rpc/evo.cpp
- doc/zmq.md
- src/wallet/coinjoin.cpp
- src/rpc/masternode.cpp
- src/coinjoin/client.cpp
- test/functional/mempool_accept.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/outputtype.hsrc/outputtype.cppsrc/wallet/test/availablecoins_tests.cppsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.hsrc/test/versionbits_tests.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/wallet/test/availablecoins_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/test/availablecoins_tests.cppsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.h
🧠 Learnings (7)
📓 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
📚 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/availablecoins_tests.cppsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/wallet/test/availablecoins_tests.cppsrc/wallet/spend.cppsrc/wallet/test/coinselector_tests.cppsrc/wallet/spend.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/wallet/test/availablecoins_tests.cppsrc/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cpp
📚 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.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/test/coinselector_tests.cppsrc/test/versionbits_tests.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/test/versionbits_tests.cpp
🧬 Code graph analysis (1)
src/wallet/spend.cpp (1)
src/wallet/coinselection.cpp (4)
Clear(491-495)Clear(491-491)result(70-70)result(174-174)
🪛 Cppcheck (2.18.0)
src/test/versionbits_tests.cpp
[error] 187-187: There is an unknown macro here somewhere. Configuration is required. If BOOST_FIXTURE_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ 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_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (25)
src/test/versionbits_tests.cpp (1)
187-187: LGTM! Valid test fixture optimization.Downgrading from
TestingSetuptoBasicTestingSetupis appropriate here since the test suite creates its own mock blockchain viaVersionBitsTesterand doesn't require the heavier blockchain initialization thatTestingSetupprovides.Based on learnings, this is a backport PR and the changes should match upstream.
src/outputtype.cpp (1)
26-28: LGTM!The addition of
OUTPUT_TYPE_STRING_UNKNOWNparsing is consistent with the existingFormatOutputTypehandling and the assertion guards inGetDestinationForKeyandAddAndGetDestinationForScript.src/outputtype.h (1)
21-24: LGTM!The change from
extern consttostatic constexpris appropriate for a compile-time constant array. This is idiomatic modern C++ and eliminates the need for a separate definition in the.cppfile.src/wallet/spend.h (1)
39-55: LGTM!The refactor to use
std::map<OutputType, std::vector<COutput>>provides cleaner extensibility for different output types. The PascalCase method naming (All(),Size(),Clear(),Erase(),Shuffle(),Add()) is consistent, and the new helper methods provide useful abstractions for coin manipulation.src/wallet/test/availablecoins_tests.cpp (2)
65-66: LGTM!The test correctly accesses
coins[OutputType::UNKNOWN]for P2PK (coinbase) outputs and uses the newSize()method. The change aligns with the new per-type bucket API.
80-80: LGTM!Correctly accesses
coins[OutputType::LEGACY]for P2PKH outputs created viaGetNewDestination().src/wallet/test/coinselector_tests.cpp (12)
88-88: LGTM!Correctly uses
coins[OutputType::LEGACY]to add coins to the per-type bucket.
309-317: LGTM!Consistent use of the new
All()andClear()methods for accessing and resetting the coin collection.
336-336: LGTM!Correctly uses
All()to access coins for selection.
363-363: LGTM!Consistent use of
Clear()method to reset the coin collection between test cases.Also applies to: 379-379
393-393: LGTM!Correctly uses
All()to access coins for pre-selection in coin control.
415-418: LGTM!Test correctly uses
Clear()andAll()methods for the knapsack solver tests.
423-426: LGTM!Consistent mechanical replacements of
all()→All()andclear()→Clear()throughout the knapsack solver test cases. The test logic remains unchanged.Also applies to: 433-436, 447-456, 460-478, 484-484
493-542: LGTM!Consistent use of
All()andClear()methods across multiple knapsack solver test scenarios.
551-629: LGTM!All test cases correctly updated to use the new
All()andClear()API methods.
636-701: LGTM!The randomness tests correctly use
All()for accessing coins andClear()for resetting the collection.
727-727: LGTM!The
ApproximateBestSubsettest correctly usesAll()to access coins.
940-941: LGTM!The Dash-specific
minimum_inputs_testcorrectly usesAll()to iterate over available coins for coin control selection.src/wallet/spend.cpp (7)
80-87: LGTM - Size() correctly aggregates across all type buckets.The method correctly iterates over the coins map and sums the sizes of all OutputType buckets.
89-97: LGTM - All() correctly aggregates coins from all type buckets.The method efficiently reserves space and aggregates coins from all OutputType buckets into a single vector.
115-125: LGTM - Shuffle() and Add() correctly implement per-type operations.Both methods correctly operate on the per-type bucket structure. Shuffle() shuffles each bucket independently, and Add() appends to the appropriate OutputType bucket.
127-136: LGTM - GetOutputType() mapping is appropriate for Dash.The function correctly maps Dash-supported output types (SCRIPTHASH, PUBKEYHASH) to LEGACY, while mapping unsupported types (e.g., SegWit witness types) to UNKNOWN. This is appropriate since Dash does not support SegWit.
260-279: LGTM - Coin classification and bucketing logic is correct.The refactored code correctly:
- Extracts the appropriate script (redeemScript for P2SH, otherwise scriptPubKey)
- Uses
Solver()to determine theTxoutType- Maps to
OutputTypeviaGetOutputType()- Adds to the appropriate type bucket via
Add()
477-495: LGTM - AttemptSelection correctly implements per-type then mixed selection strategy.The refactored logic:
- First attempts selection from each individual OutputType bucket
- Returns the best single-type solution if found
- Falls back to mixed-type selection (using
All()) only if allowedThis correctly implements the per-type bucketing strategy while maintaining the mixed-type fallback.
613-613: LGTM - Call sites correctly use the new CoinsResult API.The updates to use
All(),Size(), andShuffle()are correct and consistent with the API refactoring.Also applies to: 649-649, 653-653
| void CoinsResult::Clear() { | ||
| coins.clear(); | ||
| } | ||
|
|
||
| void CoinsResult::Erase(std::set<COutPoint>& preset_coins) | ||
| { | ||
| legacy.clear(); | ||
| other.clear(); | ||
| for (auto& it : coins) { | ||
| auto& vec = it.second; | ||
| auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);}); | ||
| if (i != vec.end()) { | ||
| vec.erase(i); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual implementation of CoinsResult::Erase in src/wallet/spend.cpp
cat -n src/wallet/spend.cpp | sed -n '99,113p'Repository: dashpay/dash
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Find where Erase() is called to understand the expected behavior
rg -n "\.Erase\(" src/wallet/spend.cpp -A 2 -B 2Repository: dashpay/dash
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Check the full context around line 637 mentioned in the review
cat -n src/wallet/spend.cpp | sed -n '630,645p'Repository: dashpay/dash
Length of output: 978
🏁 Script executed:
#!/bin/bash
# Search for class definition of CoinsResult to understand the structure
rg -B 5 -A 20 "class CoinsResult" src/wallet/ --type=cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find where preset_coins is populated (around lines 557-603 as mentioned in snippet)
cat -n src/wallet/spend.cpp | sed -n '550,605p'Repository: dashpay/dash
Length of output: 3197
🏁 Script executed:
#!/bin/bash
# Look at the CoinsResult class definition to understand the data structure
rg -B 5 -A 30 "class CoinsResult" src/wallet/ --type=cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if there's a comment or documentation about what preset_coins should contain
rg -B 3 "preset_coins" src/wallet/spend.cpp | head -40Repository: dashpay/dash
Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Get full CoinsResult class definition
rg -B 5 -A 50 "class CoinsResult" src/wallet/spend.h --type=cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check what coins is - likely a map with denomination as key
rg "std::map.*coins" src/wallet/spend.h -A 2 -B 2 --type=cppRepository: dashpay/dash
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Look for any comment or documentation about what Erase should do
git log --oneline -n 20 --all -- src/wallet/spend.cpp | head -20Repository: dashpay/dash
Length of output: 1725
🏁 Script executed:
#!/bin/bash
# Look at the git history to see when Erase was added and any related commits
git log --all --oneline --grep="Erase\|preset_coins" -- src/wallet/spend.cpp | head -20Repository: dashpay/dash
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Check the full Erase implementation with more context
rg -B 5 -A 15 "void CoinsResult::Erase" src/wallet/spend.cpp --type=cppRepository: dashpay/dash
Length of output: 592
🏁 Script executed:
#!/bin/bash
# Verify how many outpoints could potentially be in preset_coins by checking the loop
cat -n src/wallet/spend.cpp | sed -n '559,603p' | grep -E "preset_coins|for|while"Repository: dashpay/dash
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Show the bugfix commit that addresses this issue
git show f930aefff9 --statRepository: dashpay/dash
Length of output: 445
🏁 Script executed:
#!/bin/bash
# Get the full diff of the bugfix commit
git show f930aefff9Repository: dashpay/dash
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Check if the bugfix commit is in the current history/branch
git log --oneline --all -n 1 --decorate HEADRepository: dashpay/dash
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check what the current HEAD has for the Erase method
git log --all --oneline --follow -- src/wallet/spend.cpp | head -30Repository: dashpay/dash
Length of output: 2479
🏁 Script executed:
#!/bin/bash
# Get the current commit hash and see if it's before or after the bugfix
git log --all --format="%H %s" -- src/wallet/spend.cpp | grep -E "f930aefff9|bugfix.*Erase"Repository: dashpay/dash
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Check if there were any reverts or conflicts after the bugfix
git log --all --format="%H %s" -- src/wallet/spend.cpp | head -50 | tail -30Repository: dashpay/dash
Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Look for the actual method signature in the current code
rg "void CoinsResult::Erase" src/wallet/spend.h -A 1 --type=cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if maybe there's a newer version of this in a header file
fd -e h -e hpp src/wallet | xargs rg "CoinsResult" 2>/dev/nullRepository: dashpay/dash
Length of output: 3558
🏁 Script executed:
#!/bin/bash
# Check if the bugfix commit f930aefff9 is an ancestor of HEAD
git merge-base --is-ancestor f930aefff9 HEAD && echo "Bugfix IS in current branch" || echo "Bugfix NOT in current branch"Repository: dashpay/dash
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Check if there was a revert of the bugfix
git log --all --format="%H %s" --grep="Revert.*Erase\|Revert.*f930aefff9" -- src/wallet/spend.cppRepository: dashpay/dash
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the full history including any reverts or changes to Erase method
git log --all --format="%H %s" -p -- src/wallet/spend.cpp | grep -B 2 -A 10 "void CoinsResult::Erase" | head -80Repository: dashpay/dash
Length of output: 3068
🏁 Script executed:
#!/bin/bash
# Get the exact diff of how Erase was changed - look for when it went back to the broken version
git log --all --format="%H %ad %s" --date=short -p -- src/wallet/spend.cpp | grep -B 5 -A 15 "void CoinsResult::Erase.*preset_coins" | tail -100Repository: dashpay/dash
Length of output: 3080
🏁 Script executed:
#!/bin/bash
# Find all commits that touched the Erase method after the bugfix
git log --all --format="%H %ad %s" --date=short -- src/wallet/spend.cpp | head -50Repository: dashpay/dash
Length of output: 6264
🏁 Script executed:
#!/bin/bash
# Check what the actual current code is one more time to be absolutely sure
sed -n '103,112p' src/wallet/spend.cppRepository: dashpay/dash
Length of output: 387
Critical: Erase() only removes one coin instead of all matching coins.
The Erase() method has a break statement on line 110 that exits after erasing only the first matching coin. If preset_coins contains multiple outpoints, only one coin will be removed from available_coins, leaving the rest.
The expected behavior based on usage at line 637 is to remove ALL coins from available_coins that match ANY outpoint in the preset_coins set. The current implementation fails to do this.
Apply this diff to fix the logic:
void CoinsResult::Erase(std::set<COutPoint>& preset_coins)
{
for (auto& it : coins) {
auto& vec = it.second;
- auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);});
- if (i != vec.end()) {
- vec.erase(i);
- break;
- }
+ vec.erase(
+ std::remove_if(vec.begin(), vec.end(),
+ [&](const COutput &c) { return preset_coins.count(c.outpoint); }),
+ vec.end()
+ );
}
}🤖 Prompt for AI Agents
In src/wallet/spend.cpp around lines 99 to 113, Erase() currently stops after
removing the first matching coin due to the break; instead remove all coins
whose outpoint is in preset_coins. Replace the loop with logic that erases all
matching entries from each vec (for example use the erase-remove_if idiom on vec
with a predicate checking preset_coins.count(c.outpoint)), and do not break
after one deletion so every matching coin across all vectors is removed.
There was a problem hiding this comment.
Good catch! We should backport bitcoin#26560. @PastaPastaPasta
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
3 participants
Issue being fixed or feature implemented
batch of automated backports
What was done?
see commits
How Has This Been Tested?
see CI
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.