backport: merge bitcoin#24711, #24666, #25083, #25005, #25410, #24236, #25438, #25036, #25489, #25544, #25594, #13226, partial bitcoin#25218, #26005 (wallet backports: part 6) #6733
Conversation
kwvg
changed the title
WalkthroughThis set of changes introduces significant refactoring and new functionality across the wallet, coin selection, database, and testing components. The wallet code now uses unified interfaces for checking spent and locked coins via 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (47)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (38)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,h,cc,cxx,hpp}Instructions used from: Sources:
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}Instructions used from: Sources:
🧠 Learnings (3)📓 Common learningssrc/wallet/test/wallet_tests.cpp (6)src/txdb.cpp (3)⏰ 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). (6)
🔇 Additional comments (10)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Makefile.am (1)
370-370: Keep alphabetical ordering inBITCOIN_CORE_Hfor easier grepping
util/result.hwas appended to the end of the block. For long, alphabetically-sorted lists like this one it is worth inserting the path at the right spot (util/ranges.h,util/readwritefile.h, util/result.h,util/ranges_set.h, …) to avoid duplicated entries and reduce merge-conflict friction.src/qt/coincontroldialog.cpp (1)
22-23:<wallet/coinselection.h>appears to be unusedThe translation unit already included
<wallet/coincontrol.h>for the dialog’s existing logic, but I can’t find any direct usage of symbols fromwallet/coinselection.hin this file. Dropping the include avoids an extra heavyweight header and reduces rebuild fan-out.-#include <wallet/coincontrol.h> -#include <wallet/coinselection.h> +#include <wallet/coincontrol.h>If forthcoming patches do need
coinselection.h, ignore this note; otherwise please remove.test/functional/test_framework/util.py (1)
368-368: Fix comment formatting and approve legacy compatibility enhancement.The URL encoding of RPC passwords correctly addresses compatibility with legacy versions (v0.12.1.x and lower) that used URL-unsafe Base64 passwords.
Apply this diff to fix the comment formatting:
- rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote it + rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote itsrc/wallet/coinselection.h (2)
21-28: Consider the design choice of struct with private members.While the use of
std::optionalfor calculated values is appropriate, having astructwithprivatemembers is unconventional. Structs typically have public members by default. Consider either:
- Keeping it as a
classif you want private members with public accessors- Making it a true POD struct with all public members
The current approach works but may surprise other developers.
101-111: Consider safer accessor patterns.While the assertions ensure values are set, they could cause runtime crashes. Consider providing safer alternatives:
+ bool HasFee() const { return fee.has_value(); } + bool HasEffectiveValue() const { return effective_value.has_value(); } + CAmount GetFee() const { assert(fee.has_value()); return fee.value(); }This would allow callers to check before accessing, preventing potential crashes in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
ci/test/00_setup_env_native_qt5.sh(1 hunks)src/Makefile.am(1 hunks)src/bench/coin_selection.cpp(2 hunks)src/coinjoin/client.cpp(1 hunks)src/coinjoin/util.h(1 hunks)src/coins.h(0 hunks)src/dbwrapper.h(0 hunks)src/init.cpp(1 hunks)src/interfaces/chain.h(1 hunks)src/interfaces/wallet.h(2 hunks)src/node/chainstate.cpp(1 hunks)src/node/interfaces.cpp(4 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/walletcontroller.cpp(1 hunks)src/qt/walletmodel.h(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(1 hunks)src/txdb.cpp(3 hunks)src/txdb.h(1 hunks)src/util/result.h(1 hunks)src/wallet/coincontrol.cpp(1 hunks)src/wallet/coinjoin.cpp(7 hunks)src/wallet/coinselection.cpp(6 hunks)src/wallet/coinselection.h(2 hunks)src/wallet/interfaces.cpp(4 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(4 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/coins.cpp(4 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(6 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(10 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(11 hunks)src/wallet/wallet.cpp(15 hunks)src/wallet/wallet.h(5 hunks)test/README.md(1 hunks)test/functional/feature_unsupported_utxo_db.py(1 hunks)test/functional/test_framework/authproxy.py(2 hunks)test/functional/test_framework/util.py(2 hunks)test/functional/test_runner.py(1 hunks)test/get_previous_releases.py(2 hunks)
💤 Files with no reviewable changes (2)
- src/dbwrapper.h
- src/coins.h
🧰 Additional context used
🪛 Flake8 (7.2.0)
test/functional/test_framework/util.py
[error] 368-368: at least two spaces before inline comment
(E261)
🔇 Additional comments (96)
src/interfaces/chain.h (1)
131-133: Well-designed interface addition.The new
getActiveChainLocatormethod is properly documented and follows good interface design practices. The method signature is appropriate and the documentation clearly explains the expected behavior for both scenarios (block in active chain vs. ancestor lookup).src/node/interfaces.cpp (2)
736-736: Good improvement: Explicit global mutex references.The change from
LOCK(cs_main)toLOCK(::cs_main)explicitly references the globalcs_mainmutex, improving code clarity and avoiding potential ambiguity with local variables. This change is consistent across multiple methods and follows best practices.Also applies to: 761-761, 773-773, 855-855, 945-945
764-770: Correct implementation of the new interface method.The
getActiveChainLocatorimplementation is well-structured and correct:
- Proper mutex locking with
::cs_main- Appropriate error handling by returning an empty locator when block is not found
- Correct use of
chainman().ActiveChain().GetLocator(index)to generate the locator- Implementation matches the interface contract and follows existing patterns in the codebase
src/coinjoin/util.h (1)
8-8: Header inclusion is justified
CCoinControlis used as a value-type member on line 83, therefore a full definition is required. Including the header here is correct and unavoidable; a mere forward declaration would be insufficient.test/functional/test_runner.py (1)
370-373: New test added – ensure cache population cost is acceptable
feature_unsupported_utxo_db.pyis inserted near fast (<60 s) tests. If this test pulls a legacy binary and spins up an extra node, its wall-time might be significantly higher and would be better placed nearer the top-of-file “longer” section to optimise parallelisation. Consider measuring its runtime on CI and moving accordingly.test/README.md (1)
106-107: Good catch – docs now reference v0.12.1.5The example command aligns with the new functional test. No issues spotted.
src/qt/walletmodel.h (1)
37-37: LGTM: Forward declaration updated to match struct definition.The change correctly updates the forward declaration to match the
COutputrefactoring from class to struct in the coin selection codebase.src/wallet/coincontrol.cpp (1)
10-10: LGTM: Modern initialization syntax adopted.The change from parentheses to brace initialization is a good modernization that improves consistency and prevents potential narrowing conversions.
src/bench/coin_selection.cpp (2)
48-48: LGTM: COutput constructor updated for new interface.The addition of the
feesparameter with value 0 correctly adapts to the newCOutputconstructor signature. Using 0 is appropriate for benchmark scenarios.
77-77: LGTM: Consistent COutput constructor update.The
feesparameter addition maintains consistency with the updatedCOutputinterface throughout the benchmark code.src/wallet/load.cpp (1)
136-136: LGTM: Centralized wallet load notification.The addition of
NotifyWalletLoadedcentralizes wallet load notifications and eliminates code duplication. The placement after successful wallet creation and before adding to context is correct.src/qt/test/wallettests.cpp (1)
136-136: LGTM: Test updated for new scan method signature.The addition of
save_progress=falsecorrectly adapts to the extendedScanForWalletTransactionsmethod signature. Disabling progress saving is appropriate for test scenarios.src/wallet/test/coinjoin_tests.cpp (1)
152-152: LGTM: Correct addition ofsave_progressparameter for test context.The addition of
save_progress=falseto theScanForWalletTransactionscall is appropriate for test setup. Tests should not persist intermediate progress to avoid side effects, makingfalsethe correct choice here.src/wallet/test/util.cpp (1)
40-40: LGTM: Appropriate parameter addition for test utility.The
save_progress=falseparameter addition is correct for theCreateSyncedWallettest utility function. Test utilities should not save progress to maintain test isolation and avoid persistent side effects.src/wallet/rpc/backup.cpp (1)
808-808: LGTM: Correct parameter choice for import operation.The addition of
save_progress=falseis appropriate for the Electrum wallet import operation. Import operations should typically complete atomically rather than saving intermediate progress, ensuring that failed imports can be cleanly retried without partial state persistence.src/wallet/rpc/wallet.cpp (1)
425-425: LGTM! Parameter addition aligns with wallet rescan refactor.The addition of the explicit
save_progress=falseparameter is consistent with the broader wallet refactor that enhancesScanForWalletTransactionswith progress saving control during rescans.test/functional/test_framework/util.py (1)
20-20: LGTM! Import addition supports legacy compatibility.The
urllib.parseimport is correctly added to support URL encoding of RPC passwords for legacy version compatibility.src/txdb.h (1)
71-72: LGTM! Method rename improves semantic clarity.The rename from
Upgrade()toNeedsUpgrade()correctly reflects the semantic change from performing an upgrade to detecting unsupported database formats. The updated comment clearly documents the method's purpose.test/functional/test_framework/authproxy.py (2)
77-78: LGTM! Password handling improves legacy compatibility.The URL decoding of passwords before UTF-8 encoding correctly addresses compatibility with legacy versions (v0.12.1.x and lower) that used URL-unsafe Base64 passwords. The implementation properly chains the decode and encode operations.
135-136: LGTM! Parameter conversion enhances RPC compatibility.The conversion of empty parameter dictionaries to empty arrays correctly addresses compatibility with versions v0.12.2.x and lower that expect array parameters. The logic properly handles the edge case while preserving existing functionality.
ci/test/00_setup_env_native_qt5.sh (1)
17-17: LGTM! Enhanced test coverage with legacy release.The addition of "v0.12.1.5" to
PREVIOUS_RELEASES_TO_DOWNLOADcorrectly expands the test infrastructure to support testing with legacy releases, enabling verification of unsupported UTXO database format handling.src/wallet/rpc/transactions.cpp (1)
911-911: LGTM! Consistent with the wallet scanning standardization effort.The addition of the explicit
save_progress=falseparameter toScanForWalletTransactionsis appropriate for therescanblockchainRPC context. Setting progress saving to false makes sense for user-initiated manual rescans where the operation should complete atomically without intermediate progress persistence.This change aligns well with the broader wallet enhancement pattern described in the PR objectives.
src/coinjoin/client.cpp (1)
1553-1554: LGTM! Modern wallet API usage.The change correctly modernizes the coin selection to use
AvailableCoinsListUnspentwith the sameCCoinControlfilter, maintaining identical functionality while aligning with the broader wallet interface refactoring.src/node/chainstate.cpp (1)
158-162: Approve the safety-first approach to database format handling.This change correctly prioritizes safety by refusing to load unsupported database formats rather than attempting risky automatic upgrades. The explicit requirement for user reindexing (
-reindex-chainstate) provides a clear recovery path while preventing potential database corruption during automated upgrades.src/init.cpp (1)
2014-2016: LGTM! Excellent improvement to error handling and user experience.This change effectively replaces complex legacy database upgrade logic with a clean, fail-fast approach that provides clear user guidance. The immediate return with an actionable error message (
restart with -reindex-chainstate) is much better than attempting potentially risky automatic upgrades. This aligns well with the broader architectural changes to simplify chainstate database handling.test/get_previous_releases.py (2)
95-102: LGTM: SHA256 checksums added for v0.12.1.5 release.The checksums are properly formatted and cover all supported platforms for the legacy v0.12.1.5 release, enabling secure downloads for testing compatibility with older versions.
129-143: Good enhancement: Platform normalization for legacy releases.The enhanced platform mapping logic properly handles the differences between modern and legacy platform naming conventions. The Windows platform mappings and the conditional logic for tags < "v0.12.2.3" ensure backward compatibility with older release naming schemes.
src/interfaces/wallet.h (2)
15-15: LGTM: Required include for BResult type.The inclusion of
util/result.his necessary to support the newBResultreturn type used in therestoreWalletmethod.
356-356: Excellent modernization of error handling.The change from returning a raw pointer with an error output parameter to returning
BResult<std::unique_ptr<Wallet>>provides a more modern, unified approach to error handling. This eliminates the need for output parameters and makes error handling more explicit and type-safe.src/rpc/masternode.cpp (1)
134-139: Excellent refactoring to modern coin selection approach.The refactoring simplifies the masternode collateral retrieval by:
- Using
CCoinControlwithCoinType::ONLY_MASTERNODE_COLLATERALfilter directly in the constructor- Leveraging
AvailableCoinsListUnspentunder proper wallet locking- Eliminating manual vector declaration and explicit wallet locking
This approach is more consistent with the broader wallet coin selection modernization across the codebase.
src/util/result.h (1)
16-47: Excellent implementation of result/error handling utility.The
BResult<T>class provides a clean, type-safe approach to error handling:Strengths:
- Uses
std::variantfor efficient storage of either result or error- Proper move semantics in constructors and
ReleaseObj()- Clear method naming (
HasRes(),GetObj(),GetError())- Appropriate use of assertions to enforce correct usage
- Explicit
boolconversion operator for natural conditional checkingDesign considerations:
- The default constructor creates an empty error, which may be unexpected
- The class correctly prevents access to the wrong variant arm via assertions
This utility will provide a solid foundation for unified error handling across the codebase.
src/rpc/evo.cpp (2)
258-263: LGTM: Clean refactor to use AvailableCoinsListUnspent interface.The change to iterate directly over
.coinsfrom theCoinsResultreturned byAvailableCoinsListUnspentis cleaner and aligns with the wallet interface refactoring. This eliminates the need for explicit vector declarations and simplifies the coin selection logic.
784-784: LGTM: Unified coin locking interface using COutPoint.The change to use
IsLockedCoin(ptx.collateralOutpoint)with a singleCOutPointparameter is consistent with the interface unification mentioned in the refactoring. This simplifies the API by eliminating the need for separate hash and index arguments.src/qt/walletcontroller.cpp (1)
372-375: LGTM: Proper implementation of BResult interface for wallet restoration.The changes correctly implement the new
BResult<std::unique_ptr<Wallet>>interface:
Line 372: The
restoreWalletcall signature is simplified by removing the explicit error output parameter, as errors are now encapsulated in theBResultreturn type.Line 374: Error handling properly uses the
BResultconditional check andGetError()method to extract error messages when the operation fails.Line 375: Ownership transfer correctly uses
ReleaseObj()to extract the wallet object from the successfulBResult.This refactoring improves error handling consistency across the wallet interface layer.
src/wallet/rpc/coins.cpp (4)
349-349: Excellent interface unification with COutPoint.The updates to use
COutPointdirectly forIsSpent()andIsLockedCoin()improve interface consistency and code clarity by eliminating the need to pass separate transaction hash and output index parameters.Also applies to: 353-353
605-605: Good simplification of CCoinControl construction.Passing the coin type filter directly in the constructor reduces complexity compared to setting it as a separate step.
661-661: Approve the modernized coin retrieval interface.The switch from
AvailableCoins()(which fills a passed vector) toAvailableCoinsListUnspent()(which returns a struct containing the coins) follows modern C++ practices and aligns with the broader wallet refactoring.
672-672: Consistent scriptPubKey parameter usage.The direct use of
scriptPubKeyparameter forIsSpentKey()maintains consistency with the broader COutPoint and script-based interface unification.test/functional/feature_unsupported_utxo_db.py (4)
16-31: Well-structured test setup for version compatibility.The test configuration properly sets up two nodes with different versions (v0.12.1.5 and current) to test the unsupported UTXO database scenario. The version array setup is clear and appropriate.
34-40: Good adaptation for legacy RPC compatibility.Using the legacy
generateRPC instead ofgeneratetoaddressfor v0.12.1.5 compatibility shows attention to historical API differences. The test correctly validates both the block hash and UTXO set state.
42-53: Comprehensive error message validation.The test properly validates that the current node fails to start with the expected error message instructing users to restart with
-reindex-chainstate. The error message is clear and actionable for users.
55-58: Thorough recovery validation.The test confirms that the recovery mechanism works correctly by starting with
-reindex-chainstateand validating that both the block hash and UTXO set amount match the expected values, ensuring data integrity after reindexing.src/wallet/receive.cpp (4)
118-122: Effective consolidation of filter handling.The consolidation of separate
ISMINE_SPENDABLEandISMINE_WATCH_ONLYchecks into a single combined filter masked byISMINE_ALLreduces code duplication and improves maintainability.
132-135: Consistent filter pattern application.Applying the same filter consolidation pattern in
CachedTxGetDebitmaintains consistency with the credit calculation changes.
177-184: Improved code clarity with local references and COutPoint usage.Using a local reference
const CTxOut& txoutandCOutPoint(hashTx, i)for spent status checks improves readability and aligns with the codebase's move toward consistent COutPoint usage.
354-365: Enhanced readability in address balance calculation.The use of local reference
const auto& outputand direct COutPoint constructionCOutPoint(walletEntry.first, i)makes the code more readable while maintaining the same logic.src/wallet/coinselection.cpp (5)
71-71: Approve the algorithmic improvement to use explicit indexes.The change from boolean inclusion flags to explicit index vectors (
std::vector<size_t>) in the Branch and Bound algorithm improves clarity by making the selected UTXOs explicit rather than implicit.Also applies to: 89-89
158-160: Correct output construction with index-based selection.The output construction correctly uses the selected indexes to build the final result, maintaining the algorithm's correctness despite the internal representation change.
399-399: Proper adoption of COutput getter methods.The updates to use
GetEffectiveValue()andGetFee()instead of direct member access align with the new fee-aware COutput design and improve encapsulation.Also applies to: 404-404, 409-409
444-445: Consistent getter usage in waste calculation.The
GetSelectionWastefunction correctly adopts the new getter methods, maintaining consistency with the broader COutput interface changes.
114-149: Verify the backtracking logic correctness.The refactored backtracking logic with explicit indexes is more complex. The loop that restores lookahead values (
for (--utxo_pool_index; utxo_pool_index > curr_selection.back(); --utxo_pool_index)) and the exclusion shortcut condition need careful verification.#!/bin/bash # Verify that the branch and bound algorithm logic is tested adequately rg -A 10 -B 5 "SelectCoinsBnB.*test" --type cppsrc/wallet/interfaces.cpp (3)
131-131: Consistent COutPoint adoption in wallet transaction output construction.The updates to use
COutPointdirectly inMakeWalletTxOutfunctions maintain consistency with the broader wallet interface unification and improve parameter clarity.Also applies to: 142-142
274-274: Unified coin locking interface.The direct use of
COutPointparameter forIsLockedCoin()maintains consistency with the interface unification across wallet methods.
630-637: Improved error handling with BResult pattern.The update to return
BResult<std::unique_ptr<Wallet>>instead of using an explicit error output parameter improves error handling consistency. The implementation correctly wraps either the wallet result or the error in the BResult structure.src/wallet/test/wallet_tests.cpp (7)
66-66: LGTM!The addition of
NotifyWalletLoadedensures wallet load callbacks are triggered during tests, maintaining consistency with the production wallet loading process.
138-167: Well-structured test for the new save_progress functionality.The test properly verifies that:
- Block locator is not saved when
save_progress=false- Block locator is saved when
save_progress=true- The fake time mechanism allows controlled testing of progress persistence
The switch to
CreateMockWalletDatabase()improves test isolation.
191-191: Consistent parameter usage.Appropriately using
save_progress=falsefor pruning tests where progress persistence is not being tested.Also applies to: 218-218
612-612: Correct usage of the new coin selection API.The changes properly use
AvailableCoinsListUnspent(*wallet).coins.size()to get the coin count, reflecting the newCoinsResultstruct return type.Also applies to: 622-622
1012-1012: Consistent parameter addition.Correctly adds
save_progress=falseto maintain existing test behavior.
1301-1305: Proper adaptation to the new coin selection API.The code correctly uses
AvailableCoinsListUnspentand iterates over thecoinsvector from the returnedCoinsResultstruct, maintaining the same locking behavior.Also applies to: 1357-1358
1452-1452: Consistent parameter usage.Correctly adds
save_progress=falseto the rescan operation.src/wallet/coinselection.h (2)
68-83: Well-implemented constructor with fee rate calculation.The constructor correctly:
- Handles the optional fee rate parameter
- Sets fee to 0 when
input_bytes < 0(unknown size)- Calculates effective value as output value minus fee
Good defensive programming with the conditional calculation.
85-92: Good validation logic in the fee-based constructor.The assertion properly enforces the invariant that unknown input sizes (
input_bytes < 0) must have zero fees, while known sizes can have non-negative fees. The constructor delegation pattern keeps the code DRY.src/txdb.cpp (2)
33-34: Appropriate categorization of legacy database key.Moving
DB_COINSto the deprecated keys section correctly reflects that this key format is no longer supported, only detected.
56-63: Clean implementation of legacy format detection.The
NeedsUpgrade()method efficiently detects legacy UTXO entries by:
- Seeking directly to the
DB_COINSprefix- Returning immediately upon finding any such entry
- Providing clear documentation about when this format was deprecated
This is much simpler than the removed upgrade logic while serving the detection purpose.
src/wallet/spend.h (3)
28-32: Well-designed result structure.The
CoinsResultstruct cleanly encapsulates both the coins list and their total amount, providing a more cohesive API than the previous approach of modifying parameters by reference.
33-44: Excellent API improvements to AvailableCoins.The refactored function signature provides:
- Clean return type with
CoinsResult- Fee rate awareness via optional parameter
- Flexibility with
only_spendableflag- Good defaults maintaining backward compatibility
- Clear documentation of default behavior
These changes make the API more intuitive and powerful.
46-50: Useful wrapper function for coin enumeration.
AvailableCoinsListUnspentprovides a clear API for listing all coins without transaction funding context. The documentation properly explains its use case (e.g., RPC commands), and settingonly_spendable=falseensures comprehensive coin listing.src/wallet/test/coinselector_tests.cpp (5)
39-91: LGTM! Test helper functions correctly adapted for fee-aware coin selection.The updates to the
add_coinhelper functions properly incorporate the new fee parameters required by the updatedCOutputconstructor. Settinginput_bytesto 148 is appropriate for typical P2PKH inputs.
308-396: Test scenarios properly cover fee-aware coin selection logic.The updated tests correctly validate coin selection behavior under different fee rate conditions:
- Single coin selection when effective fee rate > long-term fee rate
- Multiple coin selection when effective fee rate < long-term fee rate
- Pre-selected coin behavior
Note: The comment on line 378 appropriately documents that Dash doesn't use BnB algorithm, explaining why that particular check is commented out.
420-442: Knapsack solver tests correctly updated with fee rate parameters.The test maintains backward compatibility by using
CFeeRate(0)for scenarios where fee rates aren't relevant to the test logic.
92-96: Correct usage of GetEffectiveValue() accessor method.The change from direct member access to the
GetEffectiveValue()method properly follows the updatedCOutputinterface design.
884-918: Excellent test coverage for the new GetEffectiveValue() functionality.The test comprehensively covers:
- Standard fee rate calculation (10000 - 148 = 9852)
- Unknown input size handling (returns absolute value)
- Negative effective value scenarios
- Direct fee passing vs. fee rate calculation
All expected values are correctly calculated.
src/wallet/coinjoin.cpp (5)
56-61: Clean refactoring using the new coin selection interface.The change to use
CCoinControlwithCoinType::ONLY_READY_TO_MIXandAvailableCoinsListUnspentimproves code clarity and maintainability by leveraging the centralized coin selection infrastructure.
92-108: Consistent refactoring and proper GetEffectiveValue() usage.The changes properly:
- Use
GetEffectiveValue()accessor in the comparator- Apply the same
CCoinControl/AvailableCoinspattern for consistency- Document why
CFeeRate(0)is used (effective value calculation requirement)
170-184: Proper use of COutPoint for coin state checks.The changes correctly use
COutPointobjects forIsSpent,IsLockedCoin, andIsFullyMixedchecks, aligning with the improved wallet interface that uses unified output references.
230-234: Clean refactoring of HasCollateralInputs using the coin control pattern.The function now properly uses
CCoinControlwithCoinType::ONLY_COINJOIN_COLLATERALfilter and the simplifiedAvailableCoinsListUnspentinterface.
508-508: Consistent COutPoint usage for IsSpent checks.Both instances correctly use
COutPointobjects forIsSpentchecks, maintaining consistency with the updated wallet interface.Also applies to: 548-548
src/wallet/wallet.h (4)
69-69: Good addition of wallet load notification function.The
NotifyWalletLoadedfunction follows the established pattern for wallet event notifications and will help with event-driven wallet management.
550-559: Improved method signatures using proper types.The changes to use
COutPointandCScriptparameters instead of separate hash/index parameters improve:
- Type safety by using domain objects
- Code clarity by reducing parameter count
- Consistency across the wallet interface
639-639: Useful addition of save_progress parameter to wallet scanning.The
save_progressparameter allows callers to control whether scan progress is persisted, which is valuable for different scanning scenarios (e.g., initial scan vs. rescan).
1063-1092: Excellent testability improvement for wallet rescanning.The addition of clock abstraction through:
Clocktype aliasNowFnfunction type- Injectable time function via
setNow()This follows best practices for making time-dependent code testable by allowing time injection in tests.
src/wallet/spend.cpp (8)
69-77: Function signature modernization looks goodThe transformation from a void function with output parameter to returning a
CoinsResultstruct is a clean refactoring that improves the API. The addition offeerateandonly_spendableparameters enables fee-aware coin selection, which aligns with the broader wallet modernization effort.
190-192: Fee-aware COutput construction is properly implementedThe construction includes the fee rate parameter which enables effective value calculations in coin selection algorithms. The total amount accumulation is correctly maintained.
210-213: Well-designed wrapper function for backward compatibilityThe
AvailableCoinsListUnspentwrapper provides a clean interface for callers that don't need fee-aware coin selection, maintaining backward compatibility while allowing the main function to evolve.
218-224: GetAvailableBalance correctly adapted to new interfaceThe function properly uses the new
AvailableCoinsinterface and returns thetotal_amountfield from the result structure.
257-257: ListCoins properly uses the new interfaceThe function correctly uses
AvailableCoinsListUnspent()and accesses thecoinsfield from the returnedCoinsResult.
489-494: Fee-aware coin selection properly implementedThe
COutputconstruction correctly includes the effective fee rate, and the subsequent use ofGetEffectiveValue()ensures that the effective value is calculated considering the transaction fees.
793-802: Transaction creation correctly uses fee-aware coin selectionThe call to
AvailableCoinsproperly passes the effective fee rate for fee-aware coin selection, and correctly usesonly_spendable=trueby default to ensure only spendable outputs are considered for transaction creation.
186-190: Potential inconsistency in watch-only parameter usageThe
spendablevariable already considerscoinControl && coinControl->fAllowWatchOnly && solvablein line 184, but line 189 usescoinControl && coinControl->fAllowWatchOnlydirectly. Consider using the computedspendablevalue for consistency:-int input_bytes = GetTxSpendSize(wallet, wtx, i, (coinControl && coinControl->fAllowWatchOnly)); +int input_bytes = GetTxSpendSize(wallet, wtx, i, spendable && (mine & ISMINE_WATCH_ONLY));Likely an incorrect or invalid review comment.
src/wallet/wallet.cpp (6)
185-191: Good refactoring: Centralized wallet load notifications.The new
NotifyWalletLoadedfunction properly consolidates the wallet load notification logic that was previously duplicated across multiple functions. The mutex locking ensures thread safety.
393-416: Enhanced error handling with exception safety.The try-catch wrapper properly handles exceptions during wallet restoration while preserving existing error messages. The cleanup logic correctly removes files on failure regardless of the error path.
625-640: Improved API: Using COutPoint for spent checking.The signature change from separate hash/index parameters to a single
COutPointparameter improves type safety and API consistency. The implementation correctly uses the outpoint for lookups.
877-897: Correct implementation with legacy wallet compatibility.The new
IsSpentKeyimplementation properly handles both standard destination extraction and legacy wallet key checking. The additional complexity for legacy wallets is necessary for backward compatibility.
1679-1683: Well-implemented progress persistence for wallet rescanning.The addition of the
save_progressparameter and interval-based saving logic is a good optimization. Using theWalletRescanReserver's clock abstraction ensures consistent timing throughout the rescan process.Also applies to: 1711-1752
2138-2141: Consistent UTXO handling with COutPoint objects.The UTXO loading logic correctly uses
COutPointobjects and the updatedIsSpentinterface, maintaining consistency with the API changes throughout the wallet.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/test_framework/util.py (1)
368-369: Fix inline comment spacing and approve backward compatibility enhancement.The URL encoding logic correctly addresses the compatibility issue with v0.12.1.x and lower versions that used URL-unsafe Base64 for RPC passwords. Using
safe=''ensures all potentially problematic characters are properly encoded.Fix the inline comment spacing to comply with PEP 8:
- rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote it + rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
ci/test/00_setup_env_native_qt5.sh(1 hunks)src/Makefile.am(1 hunks)src/bench/coin_selection.cpp(2 hunks)src/coinjoin/client.cpp(1 hunks)src/coinjoin/util.h(1 hunks)src/coins.h(0 hunks)src/dbwrapper.h(0 hunks)src/init.cpp(1 hunks)src/interfaces/chain.h(1 hunks)src/interfaces/wallet.h(2 hunks)src/node/chainstate.cpp(1 hunks)src/node/interfaces.cpp(4 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/walletcontroller.cpp(1 hunks)src/qt/walletmodel.h(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(1 hunks)src/txdb.cpp(3 hunks)src/txdb.h(1 hunks)src/util/result.h(1 hunks)src/wallet/coincontrol.cpp(1 hunks)src/wallet/coinjoin.cpp(7 hunks)src/wallet/coinselection.cpp(6 hunks)src/wallet/coinselection.h(2 hunks)src/wallet/interfaces.cpp(4 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(4 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/coins.cpp(4 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(6 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(10 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(11 hunks)src/wallet/wallet.cpp(15 hunks)src/wallet/wallet.h(5 hunks)test/README.md(1 hunks)test/functional/feature_unsupported_utxo_db.py(1 hunks)test/functional/test_framework/authproxy.py(2 hunks)test/functional/test_framework/util.py(2 hunks)test/functional/test_runner.py(1 hunks)test/get_previous_releases.py(2 hunks)
💤 Files with no reviewable changes (2)
- src/coins.h
- src/dbwrapper.h
✅ Files skipped from review due to trivial changes (1)
- src/coinjoin/util.h
🚧 Files skipped from review as they are similar to previous changes (40)
- src/Makefile.am
- src/wallet/coincontrol.cpp
- test/functional/test_runner.py
- src/wallet/load.cpp
- test/README.md
- src/wallet/test/coinjoin_tests.cpp
- src/qt/test/wallettests.cpp
- src/wallet/rpc/wallet.cpp
- src/txdb.h
- src/node/chainstate.cpp
- src/bench/coin_selection.cpp
- src/wallet/test/util.cpp
- test/functional/test_framework/authproxy.py
- src/qt/walletmodel.h
- src/interfaces/chain.h
- src/qt/coincontroldialog.cpp
- src/node/interfaces.cpp
- src/wallet/rpc/transactions.cpp
- src/wallet/rpc/backup.cpp
- ci/test/00_setup_env_native_qt5.sh
- src/init.cpp
- src/rpc/masternode.cpp
- src/qt/walletcontroller.cpp
- src/wallet/rpc/coins.cpp
- src/wallet/coinselection.cpp
- src/util/result.h
- test/get_previous_releases.py
- src/interfaces/wallet.h
- src/rpc/evo.cpp
- test/functional/feature_unsupported_utxo_db.py
- src/wallet/interfaces.cpp
- src/wallet/coinjoin.cpp
- src/wallet/test/wallet_tests.cpp
- src/wallet/coinselection.h
- src/wallet/receive.cpp
- src/wallet/spend.h
- src/wallet/test/coinselector_tests.cpp
- src/wallet/wallet.h
- src/coinjoin/client.cpp
- src/wallet/wallet.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/txdb.cpp (3)
Learnt from: knst
PR: dashpay/dash#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.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
src/wallet/spend.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#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.
🪛 Flake8 (7.2.0)
test/functional/test_framework/util.py
[error] 368-368: at least two spaces before inline comment
(E261)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (8)
src/txdb.cpp (3)
34-34: Good architectural decision to move deprecated constant.Moving
DB_COINSto the "Keys used in previous version" section clearly indicates its deprecated status while preserving it for legacy detection purposes.
56-63: Clean implementation of legacy database detection.The
NeedsUpgrade()method provides a simple and efficient way to detect legacy UTXO entries without the complexity of the previous upgrade logic. The implementation correctly:
- Creates a database iterator to seek deprecated
DB_COINSentries- Uses
std::make_pair(DB_COINS, uint256{})to find the first entry with the deprecated prefix- Returns a boolean indicating whether any legacy entries exist
This aligns well with the PR's goal of replacing upgrade functionality with detection and reindex requirements.
75-75: Confirms removal of legacy upgrade code.The closure of the anonymous namespace at this point, combined with the AI summary, indicates successful removal of the complex legacy
CCoinsclass and upgrade logic that was previously contained within this namespace.test/functional/test_framework/util.py (1)
20-20: LGTM: Import supports backward compatibility fix.The import of
urllib.parseis necessary for the URL encoding functionality added on line 368 to handle legacy RPC password formats.src/wallet/spend.cpp (4)
70-77: LGTM! Well-designed API improvement.The function signature refactoring improves the interface by returning a structured result instead of using out-parameters, and adds useful fee rate awareness and spendable filtering capabilities.
83-83: LGTM! Clean implementation of the refactoring.The implementation correctly:
- Uses local variables for better readability
- Implements the new spendable filter logic
- Constructs COutput with fee rate awareness
- Accumulates total amount properly
- Includes early return optimizations for efficiency
Also applies to: 119-121, 187-192, 196-198, 202-204, 207-209
211-214: LGTM! Good backward compatibility wrapper.The helper function provides a clean interface for basic coin listing while maintaining compatibility with existing code that doesn't need fee rate awareness or spendable filtering.
219-225: LGTM! Consistent adoption of the new interface.All functions have been correctly updated to use the new
CoinsResultinterface:
GetAvailableBalanceproperly extractstotal_amountListCoinsuses the appropriate helper functionSelectCoinsconstructsCOutputwith fee rate and usesGetEffectiveValue()CreateTransactionInternalpasses the effective fee rate correctlyThe changes maintain existing functionality while leveraging the improved API design.
Also applies to: 258-258, 490-490, 494-494, 794-800, 803-803
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/test_framework/util.py (1)
368-369: Fix inline comment spacing and approve the URL encoding logic.The URL encoding logic correctly addresses compatibility issues with older Dash Core versions (v0.12.1.x and lower) that used URL-unsafe Base64 for RPC passwords. However, there's a formatting issue with the inline comment spacing.
Apply this diff to fix the spacing:
- rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote it + rpc_p = urllib.parse.quote(rpc_p, safe='') # v0.12.1.x and lower used URL unsafe Base64 for their passwords, quote it
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
ci/test/00_setup_env_native_qt5.sh(1 hunks)src/Makefile.am(1 hunks)src/bench/coin_selection.cpp(2 hunks)src/coinjoin/client.cpp(1 hunks)src/coinjoin/util.h(1 hunks)src/coins.h(0 hunks)src/dbwrapper.h(0 hunks)src/init.cpp(1 hunks)src/interfaces/chain.h(1 hunks)src/interfaces/wallet.h(2 hunks)src/node/chainstate.cpp(1 hunks)src/node/interfaces.cpp(4 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)src/qt/walletcontroller.cpp(1 hunks)src/qt/walletmodel.h(1 hunks)src/rpc/evo.cpp(2 hunks)src/rpc/masternode.cpp(1 hunks)src/txdb.cpp(3 hunks)src/txdb.h(1 hunks)src/util/result.h(1 hunks)src/wallet/coincontrol.cpp(1 hunks)src/wallet/coinjoin.cpp(7 hunks)src/wallet/coinselection.cpp(6 hunks)src/wallet/coinselection.h(2 hunks)src/wallet/interfaces.cpp(4 hunks)src/wallet/load.cpp(1 hunks)src/wallet/receive.cpp(4 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/coins.cpp(4 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/spend.cpp(6 hunks)src/wallet/spend.h(1 hunks)src/wallet/test/coinjoin_tests.cpp(1 hunks)src/wallet/test/coinselector_tests.cpp(10 hunks)src/wallet/test/util.cpp(1 hunks)src/wallet/test/wallet_tests.cpp(11 hunks)src/wallet/wallet.cpp(15 hunks)src/wallet/wallet.h(5 hunks)test/README.md(1 hunks)test/functional/feature_unsupported_utxo_db.py(1 hunks)test/functional/test_framework/authproxy.py(2 hunks)test/functional/test_framework/util.py(2 hunks)test/functional/test_runner.py(1 hunks)test/get_previous_releases.py(2 hunks)
💤 Files with no reviewable changes (2)
- src/coins.h
- src/dbwrapper.h
✅ Files skipped from review due to trivial changes (4)
- src/qt/walletmodel.h
- test/functional/test_runner.py
- src/Makefile.am
- src/rpc/evo.cpp
🚧 Files skipped from review as they are similar to previous changes (38)
- src/coinjoin/util.h
- src/wallet/coincontrol.cpp
- src/wallet/test/util.cpp
- src/node/chainstate.cpp
- src/wallet/load.cpp
- src/wallet/rpc/backup.cpp
- src/qt/coincontroldialog.cpp
- src/wallet/rpc/wallet.cpp
- src/wallet/test/coinjoin_tests.cpp
- test/README.md
- src/bench/coin_selection.cpp
- src/qt/test/wallettests.cpp
- src/wallet/rpc/transactions.cpp
- src/txdb.h
- test/functional/test_framework/authproxy.py
- src/interfaces/wallet.h
- src/init.cpp
- src/interfaces/chain.h
- ci/test/00_setup_env_native_qt5.sh
- src/coinjoin/client.cpp
- src/node/interfaces.cpp
- src/wallet/coinselection.cpp
- test/get_previous_releases.py
- src/rpc/masternode.cpp
- src/util/result.h
- src/qt/walletcontroller.cpp
- src/wallet/receive.cpp
- src/wallet/rpc/coins.cpp
- test/functional/feature_unsupported_utxo_db.py
- src/wallet/interfaces.cpp
- src/wallet/coinjoin.cpp
- src/wallet/coinselection.h
- src/wallet/test/wallet_tests.cpp
- src/wallet/spend.h
- src/wallet/test/coinselector_tests.cpp
- src/wallet/spend.cpp
- src/wallet/wallet.h
- src/wallet/wallet.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
src/txdb.cpp (3)
Learnt from: knst
PR: dashpay/dash#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.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
🪛 Flake8 (7.2.0)
test/functional/test_framework/util.py
[error] 368-368: at least two spaces before inline comment
(E261)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-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: linux64_sqlite-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (4)
src/txdb.cpp (3)
34-34: LGTM: Correct placement in deprecated keys section.Moving
DB_COINSto the deprecated keys section is appropriate since it was deprecated in v0.15.0. The positioning clearly indicates this is a legacy key that might still be found in older databases.
56-63: LGTM: Clean detection logic replaces upgrade attempts.The
NeedsUpgrade()method provides a simple and effective way to detect legacy database formats without attempting to upgrade them. The implementation correctly:
- Uses the deprecated
DB_COINSkey to detect legacy entries- Provides a clear comment explaining the deprecation timeline
- Returns a boolean indicating whether legacy entries are present
This aligns well with the PR's objective to replace upgrade attempts with detection and refusal to load unsupported formats.
75-75: LGTM: Proper namespace formatting.The namespace closing marker is correctly placed and follows standard C++ formatting practices.
test/functional/test_framework/util.py (1)
20-20: LGTM: Import added for URL encoding functionality.The import is necessary for the URL encoding functionality used later in the file.
Changes to `src/wallet/rpc/spend.cpp` are not included as the `sendall` RPC has not been implemented yet.
…veral code cleanups Changes to `src/wallet/rpc/spend.cpp` are not included as the `sendall` RPC has not been implemented yet.
… does not match parameter name"
The test suite can run on Windows and improvements in the build system will allow native builds on Windows. When that happens, we need to be able to fetch old Windows releases.
Relevant for the upcoming backport, which relies on a version of Dash before v0.12.2.2 for validating migration code handling databases created before bitcoin#10195 (which is included in v0.12.2.2, so we must use one patch version earlier, i.e. v0.12.1.x)
…ower bitcoin#8811 (eb7a6b0) allowed the RPC framework to parse objects but this backport first debuted in v0.12.3, which means that nodes of even lower versions cannot make sense of the blank object sent by default and error out. We need to pass a blank array instead. This is a forward-compatible change.
Dash used URL unsafe Base64 encoding for their password field until dash#1454, included in v0.12.2.0. This unfortunately means we have to make sure we quote our URLs remaining sensitive to the behavior of older versions.
…BWrapper,CCoinsViewDBCursor`
…t to CreateTransaction and GetNewDestination includes: - 7a45c33
…reWallet, and in general via interfaces) excludes: - c3e5365
…n by index rather than by position
…fier for <v0.12.3.x and bail out on macOS, make `feature_unsupported_utxo_db` Linux-only 7ace512 fix: restrict `feature_unsupported_utxo_db` to Linux (Kittywhiskers Van Gogh) 5c38d5c fix: don't download v0.12.2.x on macOS due to missing releases (Kittywhiskers Van Gogh) 722997e fix: avoid exclusion of v0.12.2.3 from platform identifier (Kittywhiskers Van Gogh) fb2648a fix: compare against lower version first to avoid short-circuiting (Kittywhiskers Van Gogh) Pull request description: ## Motivation `feature_unsupported_utxo_db.py` (introduced with [bitcoin#24236](bitcoin#24236) as 79fcd30 in [dash#6733](#6733)) creates problems on macOS for the following reasons: * The platform identifier detection code was placed _after_ v20 when it should've been positioned _before_, as the prior condition was met, platform identifier was taken to be `osx64`, which is incorrect ([release](https://github.com/dashpay/dash/releases/tag/v0.12.1.5)). <details> <summary>Error log:</summary> ``` $ ./test/get_previous_releases.py -b -t "releases" ${PREVIOUS_RELEASES_TO_DOWNLOAD} Releases directory: releases Fetching: https://github.com/dashpay/dash/releases/download/v0.12.1.5/dashcore-0.12.1.5-osx64.tar.gz % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 9 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 9 100 9 0 0 34 0 --:--:-- --:--:-- --:--:-- 34 Checksum for given version doesn't exist ``` </details> * Dash Core v0.12.2.x and earlier did not include daemon or utility binaries for macOS (i.e. `dashd`, as needed by the functional test suite), shipping only Dash-Qt using a disk image, in comparison to v0.12.3.x which do offer them in a tarbundle ([release](https://github.com/dashpay/dash/releases/tag/v0.12.3.1)). * Dash Core v0.12.2.3 was incorrectly excluded from the old platform determination logic due to the _less than_ comparison. In light of this, `get_previous_releases.py` will now error out if attempting to download versions <v0.12.3.x as we _cannot_ obtain the binaries needed to run functional tests. Additionally, `feature_unsupported_utxo_db.py` will bail out on non-Linux platforms as previous release tests assume all the versions are populated and will otherwise cause general failure. <details> <summary>Error log:</summary> ``` $ ./test/functional/feature_unsupported_utxo_db.py 2025-07-18T10:09:06.884000Z TestFramework (INFO): PRNG seed is: 5491056264691271257 2025-07-18T10:09:06.884000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_dkrhioh0 2025-07-18T10:09:06.885000Z TestFramework (INFO): Create previous version (v0.12.1.5) utxo db 2025-07-18T10:09:06.885000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/src/dash/test/functional/test_framework/test_framework.py", line 162, in main self.run_test() File "/src/dash/./test/functional/feature_unsupported_utxo_db.py", line 35, in run_test self.start_node(0) File "/src/dash/test/functional/test_framework/test_framework.py", line 644, in start_node node.start(*args, **kwargs) File "/src/dash/test/functional/test_framework/test_node.py", line 249, in start self.process = subprocess.Popen(all_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/subprocess.py", line 1024, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib/python3.11/subprocess.py", line 1901, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: '/src/dash/releases/v0.12.1.5/bin/dashd' 2025-07-18T10:09:06.936000Z TestFramework (INFO): Stopping nodes [...] ``` </details> Special thanks to PastaPastaPasta for flagging this issue! ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 7ace512 PastaPastaPasta: utACK 7ace512 Tree-SHA512: f5dbe033264cc2978ab28419284a737ae932c2b0fcfa6a301c6e738b80e09723760d89e8b1de2ea917ef019e09d832a31b1a6c3127b3e52b65e89d457a0f91ab
3 participants
Additional Information
When backporting bitcoin#23497 (see dash#6732), some
BOOST_AUTO_TEST_SUITE_END()s were placed outside thewalletnamespace scope, which resulted in incorrect behavior when rebasing this pull request over dash#6732. This has been resolved.Backports of bitcoin#25083 and bitcoin#25005 do not include changes to
wallet/rpc/spend.cppas thesendall()RPC has not been implemented indevelop(f453998) as of this writing.Backporting bitcoin#24236 required a fair number of additional considerations, firstly because the pull request introduces a test,
feature_unsupported_utxo_db.py, that requires the last-capable of version Dash Core to generate a legacy UTXO database (the current database format debuted in dash#1701, included in v0.12.2.2) and secondly, because v0.15 is the earliest expected version of Dash Core to work smoothly with our test framework (source).As we require the last release cycle that generates the legacy UTXO database by default, we used the last version of the v0.12.1.x family, v0.12.1.5 as the old node used in
feature_unsupported_utxo_db.py. This brings unique challenges which are elaborated on below.The test framework relies on
generatetoaddress, a wallet-independent RPC call that was introduced in bitcoin#7671 to replace the old wallet-dependentgenerateRPC. It was backported as 760d58e in dash#1790 and debuted in v0.12.3. We cannot use theself.generate()helper as simply dispatches ageneratetoaddress(source), which does not exist in v0.12.1.5.We work around this by using the more archaic
self.nodes[0].rpc.generate(), in line with usage inrpc_generate.py(source).Releases before v0.12.3.x did not include the target triples in their naming convention (see tag
v0.12.3.1vsv0.12.1.5), which causes problems when fetching packages based on the targetHOSTof the build environment. This has been remedied by introducing awareness forRPi2,osxandlinux{32,64}labels.RPi2corresponds toarm-linux-gnueabihfbased on the Gitian descriptor that was removed in dash#2183 (source)This was also taken as an opportunity to introduce awareness for the
win32andwin64labels which persist even now (see tagv22.1.2)Currently, when we want to make RPC calls without arguments, a blank object is sent to by the RPC dispatcher. This is acceptable as objects are supported since bitcoin#8811 (backported as eb7a6b0 in dash#1851), included in v0.12.3.
Unfortunately, since we are dealing with a version of Dash that is ancient even by previous release standards, this prevents the dispatcher from even acknowledging the RPC server is active (see below).
Test failure:
Currently, the password used for RPC authentication in Base16 encoded. This is a result of dash#1454, which switched away from Base64. Unfortunately, this benefit only extends to v0.12.2.0 onwards, which is too new for the old node needed in our functional test.
Worse still, it was not the URL-safe variant of Base64, so attempting to establish a connection with the old node resulted in sporadic failures (see below).
Test failure:
How Has This Been Tested?
A full CoinJoin session run on 89cd497
Breaking Changes
None expected.
Checklist