refactor: consolidate ProTx wallet UTXO locking logic #6536
Conversation
|
removed for merge commit simplicity |
![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 (3)
src/wallet/wallet.h (2)
766-771: Enhance documentation for ret_dups parameter.The documentation should clarify the purpose and behavior of the
ret_dupsparameter, specifically whether it affects the function's behavior or just the return value.- /** Add new UTXOs to the wallet UTXO set - * - * @param[in] tx Transaction to scan eligible UTXOs from - * @param[in] ret_dups Allow UTXOs already in set to be included in return value - * @returns Set of all new UTXOs (eligible to be) added to set */ + /** Add new UTXOs to the wallet UTXO set + * + * @param[in] tx Transaction to scan eligible UTXOs from + * @param[in] ret_dups When true, includes UTXOs already in set in the return value. + * When false, only returns newly added UTXOs. + * @returns Set of UTXOs that were added (if ret_dups=false) or all eligible + * UTXOs from tx (if ret_dups=true) */
1047-1050: Add documentation for coin management functions.These functions lack documentation explaining their purpose, behavior, and relationships. Documentation is particularly important for the overloaded
ListProTxCoinsto explain the difference between the two variants.+ /** Lists all locked coins in the wallet. + * @param[out] outputs Vector to be populated with locked coin outpoints */ void ListLockedCoins(std::vector<COutPoint>& outputs) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Lists all ProTx coins in the wallet. + * @param[out] outputs Vector to be populated with ProTx coin outpoints */ void ListProTxCoins(std::vector<COutPoint>& outputs) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Lists ProTx coins from a specific set of UTXOs. + * @param[in] utxos Set of UTXOs to check for ProTx coins + * @param[out] outputs Vector to be populated with ProTx coin outpoints */ void ListProTxCoins(const std::set<COutPoint>& utxos, std::vector<COutPoint>& outputs) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Locks all ProTx coins from a specific set of UTXOs. + * @param[in] utxos Set of UTXOs to lock if they are ProTx coins + * @param[in] batch Optional batch to use for writing lock status to database */ void LockProTxCoins(const std::set<COutPoint>& utxos, WalletBatch* batch = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);src/wallet/wallet.cpp (1)
1040-1053: LGTM! Consider adding documentation.The implementation correctly handles UTXO set management with proper locking and duplicate handling. Consider adding documentation to explain the purpose of the
ret_dupsparameter and the function's behavior.Add documentation like:
/** * Add unspent transaction outputs from a transaction to the wallet UTXO set. * @param[in] tx Transaction to add UTXOs from * @param[in] ret_dups Whether to return duplicate UTXOs that are already in the set * @return Set of COutPoints that were added or attempted to be added (if ret_dups=true) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(2 hunks)
🔇 Additional comments (8)
src/wallet/wallet.h (1)
766-771: LGTM! The design is clean and consistent.The new UTXO and ProTx management functions are well-integrated into the CWallet class, following existing patterns and maintaining proper thread safety through consistent use of cs_wallet locks.
Also applies to: 1047-1050
src/wallet/wallet.cpp (7)
917-925: LGTM! Proper UTXO tracking for new transactions.The code correctly tracks UTXOs from newly inserted transactions, using the new AddWalletUTXOs function with ret_dups=true to get all candidates for ProTx locking.
941-942: LGTM! Proper UTXO tracking for existing transactions.The code correctly tracks UTXOs from existing transactions, using AddWalletUTXOs with ret_dups=false and properly updates the transaction status if new UTXOs are found.
945-946: LGTM! Proper ProTx locking after UTXO tracking.The code correctly locks ProTx coins using the candidates collected from UTXO tracking, maintaining transactional consistency by using the same batch.
4496-4513: LGTM! Well-designed ProTx coin listing implementation.The implementation efficiently handles ProTx coin listing with:
- Proper chain validation
- Efficient candidate pair building
- Clear separation between wallet UTXOs and explicit UTXO sets
4515-4522: LGTM! Proper implementation of ProTx coin locking.The code correctly identifies and locks ProTx coins while maintaining transactional consistency through proper batch usage.
4050-4056: LGTM! Efficient implementation of automatic masternode collateral locking.The code efficiently collects and locks masternode collaterals using the new UTXO tracking functionality while maintaining proper transaction batching.
4488-4494: LGTM! Clean implementation of locked coins listing.The implementation correctly handles locked coins listing with proper locking semantics.
src/wallet/wallet.cpp
Outdated
| if (IsMine(wtx.tx->vout[i]) && !IsSpent(hash, i)) { | ||
| setWalletUTXO.insert(COutPoint(hash, i)); | ||
| outputs.emplace_back(wtx.tx, i); | ||
| candidates.emplace(hash, i); |
There was a problem hiding this comment.
std::vector usually has significantly better performance (10x), RAM usage is also over-headed (due to bunch of small allocation 32bytes per one item somewhere randomly allocated in heap, instead consequence allocation as vector) compare to std::set.
std::set has a great usage when you combine 2 types of actions: "add" and "request". In case if you know all elements, you should use data structure with linear memory inside for better performace.
If it is a know case here to have duplicates, consider using std::vector and remove duplicates by std::sort + std::unique + std::erase.
There was a problem hiding this comment.
Have tried to prefer std::vector where I can but the places that still use std::set do so because setWalletUTXO is a std::set.
ListProTxCoins() originally iterated through setWalletUTXO and while it was generalized to accept any input, they still are std::sets as it still needs to accept setWalletUTXO. Due to the AddWalletUTXOs() > LockProTxCoins() > ListProTxCoins() call chain, the arguments bubbling up and return types are also std::sets.
I'm not very confident about a prune/deduplication function in case we change setWalletUTXO's type outright.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/rpc/evo.cpp (1)
1394-1394: LGTM! Nice simplification.The change improves efficiency by directly iterating over the
ListProTxCoins()result instead of storing it in an intermediate vector first. This eliminates unnecessary memory allocation and copying while maintaining the same functionality.Consider applying similar simplifications to other similar patterns in the codebase where intermediate vectors are used unnecessarily.
src/wallet/wallet.cpp (3)
1040-1053: Well-structured implementation of AddWalletUTXOsThe function is well-implemented with proper locking and validation. The use of structured bindings and modern C++ features is good.
Consider adding documentation comments to explain:
- The purpose of the ret_dups parameter
- The expected behavior when duplicates are found
- The relationship with setWalletUTXO
+// Adds unspent transaction outputs to the wallet's UTXO set +// @param tx The transaction to scan for UTXOs +// @param ret_dups If true, return duplicate UTXOs that are already in the set +// @return Set of COutPoints added (or found if ret_dups is true) std::set<COutPoint> CWallet::AddWalletUTXOs(CTransactionRef tx, bool ret_dups)
917-925: Good integration of UTXO management in AddToWalletThe changes properly integrate UTXO management for both new and existing transactions.
Consider adding error handling for AddWalletUTXOs failures:
- candidates = AddWalletUTXOs(wtx.tx, /*ret_dups=*/true); + try { + candidates = AddWalletUTXOs(wtx.tx, /*ret_dups=*/true); + } catch (const std::runtime_error& e) { + WalletLogPrintf("Failed to add UTXOs: %s\n", e.what()); + return nullptr; + }Also applies to: 941-942, 945-946
4497-4513: Clean implementation of ListProTxCoinsGood separation of concerns with overloaded functions and proper null checks.
Consider adding validation for the utxos parameter:
std::vector<COutPoint> CWallet::ListProTxCoins(const std::set<COutPoint>& utxos) const { AssertLockHeld(cs_wallet); + // Early return if no UTXOs to process + if (utxos.empty()) return std::vector<COutPoint>(); if (!m_chain) return std::vector<COutPoint>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/interfaces/wallet.h(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/wallet.cpp(6 hunks)src/wallet/wallet.h(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/wallet/rpcwallet.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/wallet.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (6)
src/interfaces/wallet.h (1)
148-151: LGTM! Interface design improvement.The change from output parameters to return values for
listLockedCoinsandlistProTxCoinsmethods improves the interface by:
- Making the methods more functional and easier to use
- Reducing the chance of errors from uninitialized output parameters
- Aligning with modern C++ practices
src/qt/masternodelist.cpp (1)
207-209: LGTM! Code simplification.The change improves the code by:
- Eliminating the need for an intermediate vector
- Using modern C++ range-based for loop for better readability
- Properly utilizing the updated wallet interface
src/wallet/interfaces.cpp (1)
258-264: LGTM! Clean implementation of the updated interface.The implementation:
- Properly maintains thread safety using
WITH_LOCK- Uses efficient direct return statements
- Correctly implements the updated interface methods
src/qt/coincontroldialog.cpp (1)
Line range hint
429-435: LGTM! Code simplification.The change improves the code by:
- Using direct initialization of the vector with the return value
- Making the code more concise and readable
- Properly utilizing the updated wallet interface
src/wallet/wallet.cpp (2)
4049-4055: Efficient refactoring of AutoLockMasternodeCollateralsThe implementation is more efficient using batch operations and the new AddWalletUTXOs function.
Let's verify the locking behavior:
✅ Verification successful
Implementation verified as thread-safe and efficient
The locking behavior is properly implemented with:
- Exclusive wallet locks protecting all critical operations
- Efficient batch processing for database operations
- Thread-safe ProTx coin handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential race conditions in masternode collateral locking rg -A 5 "LockProTxCoins|AutoLockMasternodeCollaterals" | grep -v "cs_wallet"Length of output: 3979
Script:
#!/bin/bash # Check ListProTxCoins implementation and locking rg -A 10 "ListProTxCoins|LockCoin" src/wallet/ # Check ProTx related definitions ast-grep --pattern 'class CWallet { $$$ ListProTxCoins($$$) { $$$ } $$$ }'Length of output: 9947
4515-4520: Concise implementation of LockProTxCoinsThe function is focused and makes good use of batch operations.
Let's verify the locking behavior is consistent:
✅ Verification successful
Locking behavior verified as consistent and thread-safe
The implementation correctly handles batch operations and maintains thread safety through proper locking mechanisms. All callers respect the required locking semantics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if LockProTxCoins is always called with a batch rg "LockProTxCoins.*batch.*nullptr"Length of output: 182
Script:
#!/bin/bash # Find all calls to LockProTxCoins rg "LockProTxCoins\(" -A 2 # Check LockCoin implementation ast-grep --pattern 'void LockCoin(const COutPoint& output, WalletBatch* batch) { $$$ }' # Check ListProTxCoins implementation ast-grep --pattern 'ListProTxCoins(const std::set<COutPoint>& utxos) { $$$ }'Length of output: 823
Script:
#!/bin/bash # Search for LockCoin implementation and usage rg "void.*LockCoin.*\(" -A 5 rg "LockCoin\(" -A 2 # Search for ListProTxCoins implementation and usage rg "ListProTxCoins.*\(" -A 5Length of output: 4619
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/node/interfaces.cpp (1)
Line range hint
789-804: Consider optimizing the collateral validation.The implementation is correct but could be optimized:
- The vector allocation could be pre-sized based on the input size
- The masternode list lookup could be skipped if the tip is null
Consider this optimization:
std::vector<COutPoint> listMNCollaterials(const std::vector<std::pair<const CTransactionRef&, uint32_t>>& outputs) override { const CBlockIndex *tip = WITH_LOCK(::cs_main, return chainman().ActiveChain().Tip()); + std::vector<COutPoint> listRet; + listRet.reserve(outputs.size()); // Pre-allocate for better performance CDeterministicMNList mnList{}; - if (tip != nullptr && m_node.dmnman != nullptr) { + if (tip != nullptr && m_node.dmnman != nullptr) { mnList = m_node.dmnman->GetListForBlock(tip); } - std::vector<COutPoint> listRet; for (const auto& [tx, index]: outputs) { COutPoint nextOut{tx->GetHash(), index}; if (CDeterministicMNManager::IsProTxWithCollateral(tx, index) || mnList.HasMNByCollateral(nextOut)) { listRet.emplace_back(nextOut); } } return listRet; }src/wallet/wallet.cpp (1)
1040-1053: Consider optimizing set operations in AddWalletUTXOsThe implementation is correct but could be optimized:
- Consider using reserve() on the return set if size is predictable
- The emplace operation could be combined with the insertion check
std::set<COutPoint> ret; +ret.reserve(tx->vout.size()); // Pre-allocate if most outputs likely belong to wallet for (size_t idx = 0; idx < tx->vout.size(); ++idx) { if (IsMine(tx->vout[idx]) && !IsSpent(hash, idx)) { - if (auto [_, inserted] = setWalletUTXO.emplace(hash, idx); inserted || ret_dups) { - ret.emplace(hash, idx); - } + if (setWalletUTXO.emplace(hash, idx).second || ret_dups) { + ret.emplace(hash, idx); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/interfaces/chain.h(1 hunks)src/interfaces/wallet.h(1 hunks)src/node/interfaces.cpp(1 hunks)src/qt/coincontroldialog.cpp(1 hunks)src/qt/masternodelist.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(1 hunks)src/wallet/wallet.cpp(6 hunks)src/wallet/wallet.h(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/wallet/rpcwallet.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/qt/masternodelist.cpp
- src/rpc/evo.cpp
- src/qt/coincontroldialog.cpp
- src/interfaces/wallet.h
- src/wallet/interfaces.cpp
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: predict_conflicts
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (8)
src/interfaces/chain.h (1)
146-146: LGTM! Type safety improvement.The change from
unsigned inttouint32_timproves type safety by using a fixed-width integer type, making the size and signedness explicit.src/wallet/wallet.h (2)
766-771: LGTM! Well-documented new method.The
AddWalletUTXOsmethod is well-documented with clear parameters and return value description. The EXCLUSIVE_LOCKS_REQUIRED annotation ensures thread safety.
1047-1050: Verify the impact of signature changes.The changes to ProTx-related methods modify their signatures to return vectors directly and add new overloads. While this is a good design improvement, we should verify all callers are updated.
Run this script to find all callers:
✅ Verification successful
All callers are compatible with the modified method signatures
The verification confirms that all callers correctly handle the vector return types and updated parameters. No changes are required in the dependent code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all callers of the modified ProTx methods echo "Searching for ListLockedCoins callers..." rg "ListLockedCoins" -A 2 echo "Searching for ListProTxCoins callers..." rg "ListProTxCoins" -A 2 echo "Searching for LockProTxCoins callers..." rg "LockProTxCoins" -A 2Length of output: 3159
src/wallet/wallet.cpp (5)
941-946: LGTM! Proper error handling for UTXO updatesThe code correctly handles UTXO updates by:
- Only collecting candidates if not empty
- Setting fUpdated flag appropriately
- Locking ProTx coins in the same batch context
4492-4508: LGTM! Clean implementation of ListProTxCoinsThe code follows good practices:
- Proper null checks for chain interface
- Efficient filtering of candidates
- Clear separation between UTXO collection and ProTx validation
4510-4515: LGTM! Proper locking of ProTx coinsThe implementation correctly:
- Takes batch parameter for transaction efficiency
- Uses ListProTxCoins for filtering
- Maintains proper locking state
4047-4054: LGTM! Simplified AutoLockMasternodeCollaterals implementationThe refactored code is cleaner and more maintainable:
- Uses new helper functions effectively
- Proper batch handling for database operations
- Clear separation of concerns
917-925: Verify transaction update logic with new UTXO collection flowThe UTXO collection has been moved before the transaction update check. While this works, we should verify that moving this operation doesn't affect transaction state consistency.
✅ Verification successful
Moving UTXO collection is safe - no state dependencies
The UTXO collection operation is independent of transaction state updates (confirm, timestamps, etc.) as it only depends on the transaction data itself. Moving it before the state updates does not affect consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing transaction update patterns that depend on the old order rg -A 5 "fUpdated.*mapWallet" --type cppLength of output: 42
Script:
#!/bin/bash # Search for AddToWallet implementations and related transaction state handling echo "=== AddToWallet implementations ===" ast-grep --pattern 'bool AddToWallet($_) { $$$ }' echo -e "\n=== Transaction state modifications ===" rg "wtx\.(m_confirm|nTimeReceived|nOrderPos|m_it_wtxOrdered|nTimeSmart)" -A 3 --type cpp echo -e "\n=== UTXO operations ===" rg "AddWalletUTXOs|mapWallet" -A 3 --type cppLength of output: 42295
4 participants
Additional Information
This pull request consolidates locking ProTx UTXOs in a wallet by splitting them into three steps
AddWalletUTXOs())ListProTxCoins()to accept supplied sets)LockProTxCoins(), which utilizes the previous two steps)These are the behavior changes expected from this PR:
AutoLockMasternodeCollaterals()will now add new UTXOs it finds to the wallet UTXO set (setWalletUTXO) (earlier behavior was to only scan and lock).LockCoin()calls)The
m_chainchecks removed from various sections are acceptable as the check is still present inListProTxCoins()(source) and it will clear the vector if it fails, which will effectively skip subsequent logic (also because it's the only place wherelistMNCollaterialsis called)ListProTxCoins()is called but this isn't undesirable.Breaking Changes
None expected.
Checklist