backport: merge bitcoin#23647, #23667 (split up rpcwallet) #6687
Conversation
WalkthroughThis change reorganizes the wallet RPC implementation by splitting large, monolithic source files into multiple modular files under ✨ 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/wallet/rpc/backup.cpp (2)
48-60:⚠️ Potential issueNon-standard hex decoding logic may produce incorrect results.
The hex decoding formula at lines 53-54 appears to be non-standard. The calculation
(((str[pos+1]>>6)*9+((str[pos+1]-'0')&15))is unusual for hex character conversion.Consider using standard hex parsing utilities:
static std::string DecodeDumpString(const std::string &str) { std::stringstream ret; for (unsigned int pos = 0; pos < str.length(); pos++) { unsigned char c = str[pos]; if (c == '%' && pos+2 < str.length()) { - c = (((str[pos+1]>>6)*9+((str[pos+1]-'0')&15)) << 4) | - ((str[pos+2]>>6)*9+((str[pos+2]-'0')&15)); + // Parse two hex characters + std::string hex = str.substr(pos+1, 2); + c = static_cast<unsigned char>(std::stoul(hex, nullptr, 16)); pos += 2; } ret << c; } return ret.str(); }
687-687: 🛠️ Refactor suggestionUse fs::u8path for consistent file path handling.
The file open operation should use
fs::u8pathfor proper UTF-8 path handling across platforms, consistent with other file operations in the codebase.Apply this diff to fix the file path handling:
- file.open(strFileName, std::ios::in | std::ios::ate); + file.open(fs::u8path(strFileName), std::ios::in | std::ios::ate);
🧹 Nitpick comments (3)
src/wallet/rpc/encrypt.cpp (1)
74-74: Fix typo in comment.The comment contains a typo: "fuly unlocked" should be "fully unlocked".
- // Downgrading from "fuly unlocked" mode to "mixing only" one is not supported. + // Downgrading from "fully unlocked" mode to "mixing only" one is not supported.src/wallet/rpc/spend.cpp (1)
252-252: Fix typo in parameter description.The parameter description contains a typo: "infomration" should be "information".
- {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If true, return extra infomration about the transaction."}, + {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If true, return extra information about the transaction."},src/wallet/rpc/wallet.cpp (1)
790-791: Note: TODO for mnemonic featureThere's a TODO comment about adding mnemonic feature to sethdseed or removing it in favor of upgradetohd. This should be addressed for API consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between da8a475 and 917c1e99fc1fc63095f657e50908b273513c8541.
📒 Files selected for processing (14)
doc/release-notes-6686.md(1 hunks)src/Makefile.am(2 hunks)src/rpc/evo.cpp(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/coins.cpp(1 hunks)src/wallet/rpc/encrypt.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/util.cpp(2 hunks)src/wallet/rpc/util.h(2 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/rpc/wallet.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🪛 GitHub Actions: Check Potential Conflicts
src/rpc/evo.cpp
[error] 1-1: Conflict detected with PR #5343. Manual resolution required.
🔇 Additional comments (38)
doc/release-notes-6686.md (1)
1-5: Documentation looks good.The release notes correctly document the removal of the deprecated
instantsendtoaddressRPC command.src/wallet/interfaces.cpp (1)
30-30: Include path update is correct.The header file path has been properly updated to reflect the new directory structure after the wallet RPC refactoring.
src/wallet/rpc/wallet.h (1)
5-6: Header guard update improves consistency.The header guard has been properly updated to
BITCOIN_WALLET_RPC_WALLET_Hto match the new file path structure. The removal of function declarations is consistent with the modularization effort where these functions have been moved to their respective specialized files.Also applies to: 14-14
src/wallet/rpc/util.h (1)
14-14: Good additions for standardized error handling.The forward declarations and the new
HandleWalletErrorfunction provide a centralized way to handle wallet-related errors consistently across the refactored RPC modules. This improves maintainability and ensures uniform error reporting.Also applies to: 16-16, 42-42
src/Makefile.am (1)
407-408: LGTM! Build system correctly updated for wallet RPC modularization.The Makefile changes properly reflect the new modular structure of wallet RPC components, splitting the monolithic implementation into focused modules for addresses, backup, coins, encryption, spending, transactions, and general wallet operations.
Also applies to: 607-615
src/rpc/evo.cpp (1)
43-43:Details
✅ Verification successful
Function rename correctly reflects wallet-specific signing.
The change from
signrawtransactiontosignrawtransactionwithwalletappropriately clarifies that this RPC requires wallet access for signing operations.Let's verify that all references to this function have been updated consistently:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old function name # and verify the new function is properly declared # Search for any remaining references to signrawtransaction (without 'withwallet') echo "Checking for old function references..." rg -w "signrawtransaction\(\)" --type cpp # Verify the new function is declared in wallet RPC files echo -e "\nVerifying new function declaration..." rg "signrawtransactionwithwallet" src/wallet/rpc/ -A 2Length of output: 1775
Function rename is fully consistent and complete.
No remaining references to the old
signrawtransaction()were found, and the newsignrawtransactionwithwalletRPC is properly declared and used in all relevant files:
src/rpc/evo.cppsrc/wallet/rpc/wallet.cppsrc/wallet/rpc/spend.cppAll looks good—approving this change.
src/wallet/rpc/util.cpp (1)
125-150: Well-structured centralized error handling for wallet operations.The
HandleWalletErrorfunction provides consistent error mapping across wallet RPC operations. The special handling where bothFAILED_NOT_FOUNDandFAILED_BAD_FORMATmap toRPC_WALLET_NOT_FOUNDis logical, as both indicate an unusable wallet state from the user's perspective.src/wallet/rpc/encrypt.cpp (3)
117-162: LGTM!The
walletpassphrasechangefunction implementation is correct with proper passphrase validation, error handling, and thread safety.
164-199: LGTM!The
walletlockfunction correctly implements wallet locking with proper state validation.
201-257: LGTM!The
encryptwalletfunction implementation is well-structured with appropriate validation, error handling, and informative user feedback about the keypool flush and HD seed generation.src/wallet/rpc/spend.cpp (2)
20-82: LGTM!The
ParseRecipientsandSendMoneyhelper functions are well-implemented with proper address validation, duplicate detection, and transaction handling.
318-1058: LGTM!The remaining RPC functions (
settxfee,fundrawtransaction,signrawtransactionwithwallet,send,walletprocesspsbt,walletcreatefundedpsbt) are properly implemented with consistent error handling, parameter validation, and wallet state management.src/wallet/rpc/transactions.cpp (2)
15-415: LGTM!The helper functions (
WalletTxToJSON,ListReceived,ListTransactions) are well-implemented with proper transaction serialization, filtering logic, and edge case handling for coinbase transactions and watch-only addresses.
270-927: LGTM!The remaining RPC functions are properly implemented with consistent patterns for wallet synchronization, parameter validation, and error handling. The
rescanblockchainfunction includes appropriate checks for pruned nodes and concurrent rescans.src/wallet/rpc/coins.cpp (9)
17-76: LGTM!The
GetReceivedhelper function correctly implements the logic for calculating received amounts by address or label, with proper handling of minimum confirmations, InstantSend locked transactions, and immature coinbase outputs.
78-117: LGTM!The RPC implementation follows the standard pattern with proper parameter validation, synchronization, and error handling.
119-158: LGTM!The implementation correctly handles label-based queries with the same robustness as the address-based variant.
160-219: LGTM!The function correctly implements balance calculation with proper backward compatibility for the dummy parameter and appropriate handling of all filtering options.
221-242: LGTM!The deprecated function is properly marked and provides a simple wrapper for backward compatibility.
244-382: LGTM!The implementation is well-structured with comprehensive validation, atomic operations, and proper handling of both persistent and non-persistent locks. The two-phase approach (validation then application) ensures consistency.
384-430: LGTM!Simple and correct implementation for listing locked outputs.
432-499: LGTM!The function provides comprehensive balance information including Dash-specific CoinJoin balances and proper handling of the avoid_reuse feature.
501-733: LGTM!Comprehensive implementation with excellent parameter validation, detailed output information, and proper integration of Dash-specific features like CoinJoin rounds.
src/wallet/rpc/wallet.cpp (7)
38-43: LGTM!The helper function correctly handles checking for both compressed and uncompressed versions of a key.
90-119: LGTM!The CoinJoin rounds configuration correctly validates the input range and updates the global settings.
121-150: LGTM!The CoinJoin amount configuration properly validates the range and updates the settings.
152-273: LGTM!Comprehensive wallet information retrieval with proper handling of HD chains, scanning status, and backward compatibility for deprecated fields.
397-403: Note: TODO comment for future versionThe code contains a TODO comment about breaking changes for v21. This should be tracked for future implementation.
442-485: LGTM!The function correctly implements wallet loading with proper error handling using the new
HandleWalletErrorutility.
555-644: LGTM!Comprehensive wallet creation with proper validation of all parameters, conditional compilation checks, and appropriate setup of wallet features.
src/wallet/rpc/addresses.cpp (7)
16-56: LGTM!The function correctly implements address generation with proper capability checks and label handling.
58-90: LGTM!Correctly implements change address generation for raw transaction use cases.
92-128: LGTM!The function properly handles label assignment with correct purpose determination based on address ownership.
189-261: LGTM!Well-implemented multisig address creation with proper validation of inputs and comprehensive output including the descriptor.
338-396: LGTM!Well-structured visitor pattern implementation for address description with proper handling of different address types and multisig scripts.
398-534: LGTM!Excellent implementation providing comprehensive address information with proper error handling, HD metadata, and forward-compatible label array structure.
568-584: Good performance optimization!The use of
__pushKVinstead ofpushKVavoids O(N) key existence checks, providing O(1) insertion. The duplicate checking with std::set is a good defensive programming practice.src/wallet/rpc/backup.cpp (1)
2039-2123: Well-implemented backup and restore functionality.The new
backupwalletandrestorewalletRPC commands are well-structured with proper error handling, parameter validation, and consistent use of utility functions likeHandleWalletError.
src/wallet/rpc/transactions.cpp
Outdated
There was a problem hiding this comment.
Fix syntax error in RPC example.
The RPC example is missing a comma after "false", which would cause a parsing error.
- + HelpExampleRpc("listreceivedbyaddress", "6, false true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true")
+ + HelpExampleRpc("listreceivedbyaddress", "6, false, true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + HelpExampleRpc("listreceivedbyaddress", "6, false true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true") | |
| HelpExampleRpc("listreceivedbyaddress", "6, false, true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true") |
🤖 Prompt for AI Agents
In src/wallet/rpc/transactions.cpp at line 250, the RPC example string is
missing a comma after "false", which would lead to a syntax error. Add a comma
after "false" to separate the arguments correctly in the example string,
ensuring proper parsing of the RPC call.
src/wallet/rpc/wallet.cpp
Outdated
There was a problem hiding this comment.
Fix the RPCResult type declaration
The RPCResult declares an array type, but the function returns a JSON object with addresses as keys.
- RPCResult{
- RPCResult::Type::ARR, "", "",
- {
- {RPCResult::Type::STR_AMOUNT, "amount", "The Dash address and the amount in " + CURRENCY_UNIT},
- }
- },
+ RPCResult{
+ RPCResult::Type::OBJ, "", "",
+ {
+ {RPCResult::Type::STR_AMOUNT, "address", "The amount in " + CURRENCY_UNIT},
+ }
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RPCResult{ | |
| RPCResult::Type::ARR, "", "", | |
| { | |
| {RPCResult::Type::STR_AMOUNT, "amount", "The Dash address and the amount in " + CURRENCY_UNIT}, | |
| } | |
| RPCResult{ | |
| RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::STR_AMOUNT, "address", "The amount in " + CURRENCY_UNIT}, | |
| } | |
| }, |
🤖 Prompt for AI Agents
In src/wallet/rpc/wallet.cpp between lines 52 and 56, the RPCResult is declared
as an array type but the function actually returns a JSON object with addresses
as keys. Change the RPCResult type from ARR to OBJ to correctly represent the
returned data structure. Update the declaration to reflect that the result is a
JSON object instead of an array.
See prior commits
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/wallet/rpc/wallet.cpp (1)
52-56:⚠️ Potential issueFix the RPCResult type declaration.
The RPCResult declares an array type, but the function returns a JSON object with addresses as keys (confirmed by line 78 returning
UniValue::VOBJ).Apply this diff to fix the RPCResult type:
- RPCResult{ - RPCResult::Type::ARR, "", "", - { - {RPCResult::Type::STR_AMOUNT, "amount", "The Dash address and the amount in " + CURRENCY_UNIT}, - } - }, + RPCResult{ + RPCResult::Type::OBJ, "", "", + { + {RPCResult::Type::STR_AMOUNT, "address", "The amount in " + CURRENCY_UNIT}, + } + },src/wallet/rpc/transactions.cpp (1)
250-250: Fix syntax error in RPC example.The RPC example is missing a comma after "false", which would cause a parsing error.
- + HelpExampleRpc("listreceivedbyaddress", "6, false true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true") + + HelpExampleRpc("listreceivedbyaddress", "6, false, true, true, \"" + EXAMPLE_ADDRESS[0] + "\", true")
🧹 Nitpick comments (3)
src/wallet/rpc/wallet.cpp (1)
605-616: Improve error message clarity for descriptor wallet requirements.The error message correctly identifies the requirement but could be more user-friendly by explaining the rationale.
Consider enhancing the error message:
- throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires specifying the 'load_on_startup' flag when creating descriptor wallets. Dash Core v21 introduced this requirement due to breaking changes in the createwallet RPC."); + throw JSONRPCError(RPC_INVALID_PARAMETER, "The createwallet RPC requires explicitly specifying the 'load_on_startup' flag when creating descriptor wallets. This parameter became mandatory in Dash Core v21 to ensure consistent wallet loading behavior across restarts.");src/wallet/rpc/coins.cpp (2)
638-646: Improve type validation for coinType parameter.Consider adding explicit validation for the coinType parameter to ensure it's within the valid enum range.
if (options.exists("coinType")) { int64_t nCoinType = options["coinType"].get_int64(); if (nCoinType < static_cast<int64_t>(CoinType::MIN_COIN_TYPE) || nCoinType > static_cast<int64_t>(CoinType::MAX_COIN_TYPE)) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid coinType selected. Available range: %d - %d", static_cast<int64_t>(CoinType::MIN_COIN_TYPE), static_cast<int64_t>(CoinType::MAX_COIN_TYPE))); } + // Ensure the value is a valid CoinType enum + try { + static_cast<CoinType>(nCoinType); + } catch (...) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid coinType value"); + } + coinControl.nCoinType = static_cast<CoinType>(nCoinType); }
705-714: Optimize mempool ancestor data retrieval.The mempool ancestor information is retrieved for every unconfirmed output, which could impact performance for wallets with many unconfirmed transactions.
Consider batching mempool queries or caching this information if this becomes a performance bottleneck for large wallets with many unconfirmed outputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 917c1e99fc1fc63095f657e50908b273513c8541 and 730ef82.
📒 Files selected for processing (13)
src/Makefile.am(2 hunks)src/rpc/evo.cpp(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpc/addresses.cpp(1 hunks)src/wallet/rpc/backup.cpp(1 hunks)src/wallet/rpc/coins.cpp(1 hunks)src/wallet/rpc/encrypt.cpp(1 hunks)src/wallet/rpc/spend.cpp(1 hunks)src/wallet/rpc/transactions.cpp(1 hunks)src/wallet/rpc/util.cpp(2 hunks)src/wallet/rpc/util.h(2 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/rpc/wallet.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/wallet/interfaces.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/wallet/rpc/util.h
- src/wallet/rpc/wallet.h
- src/wallet/rpc/util.cpp
- src/wallet/rpc/backup.cpp
- src/Makefile.am
- src/rpc/evo.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code Graph Analysis (2)
src/wallet/rpc/encrypt.cpp (1)
src/wallet/rpc/util.cpp (2)
GetWalletForJSONRPCRequest(54-77)GetWalletForJSONRPCRequest(54-54)
src/wallet/rpc/addresses.cpp (3)
src/wallet/walletdb.h (1)
pwallet(235-235)src/wallet/interfaces.cpp (1)
spk_man(202-209)src/wallet/wallet.h (2)
kpSize(744-744)purpose(764-764)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (27)
src/wallet/rpc/encrypt.cpp (2)
73-78: LGTM! Excellent security logic for unlock mode transitions.The validation prevents downgrading from "fully unlocked" to "mixing only" mode, which is correct behavior. This prevents potential security issues where a user might accidentally reduce wallet capabilities.
97-110: LGTM! Proper deadlock prevention and resource management.The code correctly releases the wallet lock before scheduling the relock callback and uses weak_ptr to handle wallet unloading gracefully. The callback validation ensures only the most recent relock timer is honored.
src/wallet/rpc/wallet.cpp (1)
728-758: LGTM! Robust progress tracking and error handling.The wipewallettxes implementation correctly handles batch processing with progress updates, proper error handling for database operations, and graceful handling of user aborts. The progress calculation and batch sizing logic are well-designed.
src/wallet/rpc/coins.cpp (2)
17-76: LGTM! Comprehensive and secure implementation of GetReceived.The function correctly handles both address-based and label-based queries with proper validation, thread safety via wallet locking, and comprehensive filtering logic for various transaction types including InstantSend and coinbase handling.
316-364: LGTM! Thorough validation before coin state changes.The validation logic correctly checks transaction existence, output bounds, spent status, and current lock state before making any changes. This prevents inconsistent states and provides clear error messages.
src/wallet/rpc/addresses.cpp (10)
16-56: LGTM! Well-implemented address generation RPC.The function follows established patterns with proper wallet locking, parameter validation, capability checks, and error handling. The implementation correctly uses
GetNewDestinationand handles the case where the wallet has no available keys.
58-90: LGTM! Correct change address generation implementation.The function properly implements change address generation with appropriate parameter validation and error handling. The use of
CanGetAddresses(true)andGetNewChangeDestinationis correct for change address purposes.
92-128: LGTM! Proper label setting implementation.The function correctly validates the address, determines ownership using
IsMine(), and sets appropriate purpose labels ("receive" for owned addresses, "send" for others). Error handling for invalid addresses is properly implemented.
130-187: LGTM! Well-structured address groupings implementation.The function properly ensures chain synchronization before querying, correctly iterates through address groupings and balances, and handles optional address book labels appropriately. The nested loop structure and JSON response format are well-implemented.
189-261: LGTM! Comprehensive multisig address creation implementation.The function properly handles multisignature address creation with appropriate dual locking, parameter validation for both hex keys and addresses, and correct descriptor generation. The logic for detecting hex vs address format and the use of
AddAndGetMultisigDestinationis well-implemented.
263-305: LGTM! Secure keypool refill implementation.The function includes proper security checks (private keys enabled, wallet unlocked), validates parameters correctly (non-negative size), and verifies the operation succeeded with appropriate error handling. The HD wallet consideration in size validation is a nice touch.
307-336: LGTM! Appropriate implementation for keypool regeneration.The function correctly implements the potentially destructive keypool regeneration with proper warnings in the documentation. The use of
EnsureLegacyScriptPubKeyMan(*pwallet, true)with the safety parameter is appropriate for this operation.
338-396: LGTM! Well-designed visitor pattern for address description.The
DescribeWalletAddressVisitorclass effectively uses the visitor pattern to provide comprehensive address information. The multisig handling in theScriptHashoperator is particularly well-implemented, extracting detailed information including public keys, addresses, and signature requirements.
398-534: LGTM! Comprehensive address information implementation.The function provides thorough address analysis with proper validation, wallet type handling (legacy vs descriptor), and extensive metadata extraction including HD key information, timestamps, and labels. The error handling for invalid addresses and the comprehensive JSON response structure are well-implemented.
536-642: LGTM! Well-implemented label management functions.Both
getaddressesbylabel()andlistlabels()are properly implemented. The duplicate prevention logic usingstd::setingetaddressesbylabel()is a good optimization, and the use of__pushKVafter duplicate checking is efficient. Thelistlabels()function correctly delegates to the wallet'sListAddrBookLabelsmethod.src/wallet/rpc/spend.cpp (8)
20-82: LGTM! Well-designed helper functions with proper security checks.The
ParseRecipients()function correctly validates addresses and prevents duplicate destinations, whileSendMoney()includes essential security checks for private key availability and proper transaction creation flow. The CoinJoin handling and verbose output options are well-implemented.
97-117: LGTM! Robust fee estimation parameter handling.The function properly validates conflicting fee estimation parameters and includes appropriate precision handling for fee rates (3 significant digits). The error messages are clear and the validation logic is comprehensive.
119-221: LGTM! Comprehensive single-address sending implementation.The function includes proper chain synchronization, extensive parameter validation, and comprehensive feature support including CoinJoin, fee estimation, and address reuse avoidance. The documentation is thorough and the implementation follows security best practices.
223-316: LGTM! Well-implemented multi-recipient sending.The function properly handles multiple recipients with the same security standards as
sendtoaddress(). The backward compatibility handling for the dummy parameter and comprehensive parameter validation are well-implemented. The use of shared helper functions maintains code consistency.
318-357: LGTM! Proper fee rate validation and setting.The function includes comprehensive validation against relay minimums, wallet minimums, and maximums with clear error messages. The logic for handling automatic fee selection (rate of 0) is correct.
359-570: LGTM! Comprehensive transaction funding implementation.The
FundTransaction()helper handles complex funding scenarios with extensive parameter validation and option processing. Thefundrawtransaction()RPC wrapper correctly delegates to the helper with proper type checking. The backward compatibility handling and comprehensive options support are well-implemented.
572-661: LGTM! Secure transaction signing implementation.The function properly handles raw transaction signing with comprehensive validation, coin fetching, previous output parsing, and secure signing using the wallet's
SignTransaction()method. The error handling and result formatting are well-implemented.
663-1058: LGTM! Comprehensive PSBT and advanced sending support.The three functions (
send(),walletprocesspsbt(),walletcreatefundedpsbt()) provide comprehensive support for PSBT workflows and advanced sending options. The parameter validation is extensive, error handling is appropriate, and the implementations correctly handle the complex PSBT creation, funding, and signing workflows. The experimental warning forsend()is appropriate given its comprehensive nature.src/wallet/rpc/transactions.cpp (4)
15-57: Well-implemented transaction serialization function.The
WalletTxToJSONfunction properly handles wallet locking, extracts comprehensive transaction information, and serializes it to JSON format. The logic for handling confirmed vs unconfirmed transactions and the inclusion of Dash-specific features like InstantSend and ChainLock is appropriate.
70-211: Comprehensive and well-structured transaction listing function.The
ListReceivedfunction implements robust filtering and aggregation logic for received transactions. It properly handles various edge cases including locked transactions, watch-only addresses, coinbase exclusion, and both address and label-based grouping. The parameter validation and error handling are appropriate.
324-415: Sophisticated transaction categorization logic.The
ListTransactionsfunction implements comprehensive transaction categorization with proper handling of sent/received transactions, CoinJoin detection, coinbase maturity states, and Dash-specific platform transfers. The filtering logic for labels and watch-only addresses is correctly implemented.
439-927: Well-implemented wallet transaction RPC commands.The RPC command implementations (
listtransactions,listsinceblock,gettransaction,abandontransaction,rescanblockchain) follow proper patterns with appropriate parameter validation, wallet locking, error handling, and comprehensive help documentation. The logic for each command is correctly implemented according to their respective functionalities.
| // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed | ||
| SecureString strWalletPass; | ||
| strWalletPass.reserve(100); | ||
| // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) | ||
| // Alternately, find a way to make request.params[0] mlock()'d to begin with. | ||
| strWalletPass = request.params[0].get_str().c_str(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the security concern with passphrase handling.
The TODO comment highlights a legitimate security issue. The passphrase is temporarily stored in unprotected memory via request.params[0].get_str() before being copied to a SecureString. This creates a brief window where sensitive data could be exposed.
Consider implementing a secure parameter extraction function to minimize exposure of sensitive data in memory.
🤖 Prompt for AI Agents
In src/wallet/rpc/encrypt.cpp around lines 50 to 55, the passphrase is extracted
using request.params[0].get_str().c_str(), which temporarily stores it in
unprotected memory. To fix this, implement a secure parameter extraction method
that directly initializes the SecureString without creating an intermediate
std::string, thereby minimizing the exposure of sensitive data in unprotected
memory. Replace the current assignment with this secure extraction to avoid
using .c_str() on a temporary string.
|
Thanks! glad to see this one done. |
Additional Information
Depends on chore: drop deprecated RPC
instantsendtoaddress#6686bitcoin#23667 was merged upstream in v0.23. As of this writing, the backport backlog for v0.23 is at ~90% completion. While this refactor is disruptive, delaying it makes any backport after v0.23 at best annoying and at worst, causing history inconsistency, which may cause conflicts.
While the commits for bitcoin#23647 have been squashed down, due to the diff size, bitcoin#23667 is available commit-by-commit. Both can be validated as primarily move-only with
git log -p -n1 --color-moved=dimmed_zebra.In bitcoin#23647,
LoadWalletHelperwas moved towallet/rpc/util.cppbut bitcoin#23721 (included in dash#6137) replaced it withHandleWalletError. Due to this OOO backport, changes in bitcoin#23647 adopts portions of bitcoin#23721 for correctness and consistency.bitcoin#23667 does not move
walletdisplayaddressas it is not implemented yet on account of relying on external signer support (which is the same reason why bitcoin#23172 hasn't been backported per the spreadsheet andFundTxDocwasn't moved in this PR as it isn't backported yet).This needs to be kept in mind when backporting bitcoin#23172 and bitcoin#21467 (and its predecessors).
Breaking Changes
None expected. All changes are move-only.
Checklist