perf: wallet balance in dash-qt#6549
Conversation
WalkthroughThe pull request consolidates and revises how wallet balances are managed across several modules. Specific virtual methods for retrieving anonymized and denominated balances have been removed from the wallet interface. In their place, new member variables have been added to the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 5
🧹 Nitpick comments (5)
src/wallet/wallet.h (1)
1147-1148: Consider returning a struct for consistency
GetBalancereturns a comprehensiveBalancestructure, whileGetBalanceAnonymizedreturns justCAmount. For a more uniform interface, consider returningBalanceAnonymizedhere too, especially if multiple anonymized balance details are needed.- CAmount GetBalanceAnonymized(const CCoinControl& coinControl) const; + BalanceAnonymized GetBalanceAnonymized(const CCoinControl& coinControl) const;src/wallet/wallet.cpp (4)
Line range hint
2301-2318: Clarify early returns on coinbase and negative depth.The function correctly excludes coinbase rewards that are not yet mature or have negative depth. However, consider clarifying in the comments that coinbase transactions are skipped even if they may be partially spendable in some contexts. This can help avoid confusion for maintainers reading the code.
2335-2351: Check negative depth and uncoinfirmed logic.Early returns for negative depths and coinbase conditions are well-placed. Consider adding a brief comment explaining that unconfirmed but trusted transactions might still provide an anonymized amount if appropriately validated, to document behavior for future maintainers.
2366-2375: Guard conditions and value accumulations are appropriate.Good check for spent outputs and denominated amounts before adding anonymized credit. Ensure that partial mixing states are handled correctly: for example, if an output is half-mixed or unexpectedly marked. If partial states occur, consider an additional check or an informative comment.
2533-2544: Review coin loop performance with selected coin control.This method iterates each spendable transaction, filtering by coin control if needed. In high-transaction-volume scenarios, confirm that the approach remains performant. Caching partial results or indexing might help if further slowdown is observed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2f15c2b and 4c474c56492e7bddbbcfbecbe697f77296102f0f.
📒 Files selected for processing (5)
src/interfaces/wallet.h(1 hunks)src/qt/overviewpage.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(2 hunks)
⏰ 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 (7)
src/wallet/wallet.h (1)
531-532: Confirm concurrency handling or add lock annotationsThese methods are flagged with
NO_THREAD_SAFETY_ANALYSIS. Please verify that calling code properly lockscs_walletor otherwise ensures thread safety. If concurrency handling is required, consider adding the appropriateEXCLUSIVE_LOCKS_REQUIREDannotation.src/wallet/wallet.cpp (6)
2319-2319: Ensure coin selection logic is consistent with user expectations.When
coinControl.HasSelected()is true but a given output isn't selected, the code unconditionally skips the output. Confirm that this behavior aligns with user expectations in all usage scenarios—especially where partial selection might be relevant.
2352-2356: Validate cached logic consistency.Here, the code reuses cached anonymized credit values. Ensure that caching remains valid if external events (e.g., a coin join cycle) alter relevant outputs. Re-check concurrency safety if modifications can happen in parallel threads.
2361-2365: Simple outpoint construction is correct.Creating the
COutPointfrom the transaction hash and output index is straightforward with no apparent issues. Keep an eye on whether large transaction sizes might affect iteration performance, although that’s typically negligible here.
2379-2385: Cache assignment for anonymized amounts.Storing results in
m_amounts[ANON_CREDIT]andm_amounts[DENOM_CREDIT](orDENOM_UCREDIT) is clean. Just confirm that the concurrency model ensures no invalidation of these cached fields during updates, or thatMarkDirty()calls happen elsewhere when needed.
Line range hint
2545-2567: Balance calculation structure is clear.This logic merges trusted and untrusted pending balances while also including immature coins. Check that newly introduced fields (e.g., anonymized balance) won’t break older code that expects a simpler structure. Possibly add unit tests verifying that partial trust scenarios remain correct.
2568-2574: Ensure correct accumulation of anonymized values.Appending the anonymized sums and categorizing them into trusted vs. untrusted pending is correct. As the function interacts with coinjoin data, ensure that coinjoin state changes won’t leave stale or incorrectly classified data behind.
knst
force-pushed
the
perf-wallet-balance
branch
from
4c474c5 to
3b2c5c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/wallet/wallet.cpp (2)
Line range hint
2301-2334: Improved type safety by using reference instead of pointer.The change from
CCoinControl*toCCoinControl&eliminates potential null pointer issues and improves type safety, while maintaining the same functionality.Consider adding a comment explaining why the null check was removed since it's now enforced by the type system:
- if (coinControl.HasSelected() && !coinControl.IsSelected(outpoint)) { + // Using reference instead of pointer guarantees non-null coinControl + if (coinControl.HasSelected() && !coinControl.IsSelected(outpoint)) {
2335-2386: Well-designed balance caching mechanism with proper state tracking.The new GetAnonymizedBalance implementation introduces a robust caching system that:
- Properly tracks both anonymized and denominated amounts
- Handles unconfirmed balances separately
- Uses proper range checking with MoneyRange
Consider adding cache invalidation when transaction states change (e.g. confirmation status) to ensure cache coherency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 4c474c56492e7bddbbcfbecbe697f77296102f0f and 3b2c5c337758b05433930169e1a56bbad7834038.
📒 Files selected for processing (5)
src/interfaces/wallet.h(1 hunks)src/qt/overviewpage.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/wallet.cpp(5 hunks)src/wallet/wallet.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/qt/overviewpage.cpp
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build (linux64_ubsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_tsan, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_sqlite, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_nowallet, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_fuzz, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64_cxx20, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (linux64, x86_64-pc-linux-gnu, linux64)
- GitHub Check: Build (arm-linux, arm-linux-gnueabihf, arm-linux)
🔇 Additional comments (7)
src/interfaces/wallet.h (1)
385-386: LGTM!The new fields
denominated_untrusted_pendinganddenominated_trustedare properly added to theWalletBalancesstruct with correct initialization to track denominated transaction balances.src/wallet/interfaces.cpp (2)
395-396: LGTM!The new balance fields are properly initialized from the wallet's balance data.
428-428: LGTM!Simplified the
GetBalancecall by removing the unnecessary parameter while maintaining functionality.src/wallet/wallet.h (3)
523-529: LGTM!The
BalanceAnonymizedstruct is well-designed to encapsulate anonymized balance information with proper field initialization.
531-532: LGTM!The new methods
GetAnonymizedCreditandGetAnonymizedBalanceprovide a clean interface for retrieving anonymized balance information.
1147-1148: LGTM!The modified
GetBalanceand newGetBalanceAnonymizedmethod signatures align well with the interface changes.src/wallet/wallet.cpp (1)
Line range hint
2545-2577: Improved balance calculation with proper handling of anonymized funds.The GetBalance method has been enhanced to properly track:
- Anonymized balances using the new caching system
- Denominated amounts for both trusted and untrusted pending transactions
Let's verify the balance calculation changes:
✅ Verification successful
Verified: Balance Calculation Changes Confirmed
The grep results confirm that the updated
GetBalancemethod insrc/wallet/wallet.cppis the sole implementation handling anonymized funds via caching. All callers—such as those in CoinJoin, RPC wallet, and wallet interfaces—are properly using this updated method.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other balance calculation methods that may need similar updates rg -A 5 "GetBalance.*\(" --type cppLength of output: 9036
|
pls see 66089d0e65023b14489412a1bd75742c7212db97 and c32ca0bf33b048001f7186f984ca76644b463dd2 |
knst
force-pushed
the
perf-wallet-balance
branch
from
3b2c5c3 to
312319f
Compare
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
Seems to be working correctly, light ACK 8822b51984eb4ee3a8151248a12febdc5e6a48ea
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 123f1931827105ef7c0dd08dc2a2fe66725042c9
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
perf-wallet-balance
branch
from
123f193 to
8c54133
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/wallet/transaction.h (1)
303-304: Consider documenting thread safety requirements.Both methods are marked with
NO_THREAD_SAFETY_ANALYSIS. While there's a TODO comment about this elsewhere in the file, it would be helpful to document the actual thread safety requirements for these specific methods.Add documentation comments:
+ /** Get anonymized credit amount. Requires cs_wallet lock. */ CAmount GetAnonymizedCredit(const CCoinControl& coinControl) const NO_THREAD_SAFETY_ANALYSIS; + /** Get available CoinJoin credits. Requires cs_wallet lock. */ CoinJoinCredits GetAvailableCoinJoinCredits() const NO_THREAD_SAFETY_ANALYSIS;src/interfaces/wallet.h (1)
385-386: Consider adding documentation for the new balance fields.While the field names are descriptive, adding documentation comments would help clarify their purpose and when they are used.
Add documentation:
+ /** Balance of denominated funds that are pending confirmation */ CAmount denominated_untrusted_pending = 0; + /** Balance of denominated funds that are confirmed */ CAmount denominated_trusted = 0;src/wallet/receive.cpp (1)
353-361: Consider extracting CoinJoin balance logic to a separate method.The CoinJoin balance calculation logic inside GetBalance could be moved to a separate private method to improve readability and maintainability.
+private: + void UpdateCoinJoinBalances(const CWalletTx& wtx, Balance& ret) const { + const auto balance_anonymized = wtx.GetAnonymizedBalance(); + ret.m_anonymized += balance_anonymized.m_anonymized; + if (balance_anonymized.is_unconfirmed) { + ret.m_denominated_untrusted_pending += balance_anonymized.m_denom_credit; + } else { + ret.m_denominated_trusted += balance_anonymized.m_denom_credit; + } + }Then in GetBalance:
if (cj_enabled) { - const auto balance_anonymized = wtx.GetAnonymizedBalance(); - ret.m_anonymized += balance_anonymized.m_anonymized; - if (balance_anonymized.is_unconfirmed) { - ret.m_denominated_untrusted_pending += balance_anonymized.m_denom_credit; - } else { - ret.m_denominated_trusted += balance_anonymized.m_denom_credit; - } + UpdateCoinJoinBalances(wtx, ret); }src/wallet/wallet.cpp (1)
2047-2053: Consider initializing cache fields explicitly.The cache fields are checked before being set. Consider initializing them explicitly in the constructor to avoid potential undefined behavior.
- CWalletTx::CoinJoinCredits ret{0, false}; + CWalletTx::CoinJoinCredits ret; + ret.m_anonymized = 0; + ret.m_denominated = 0; + ret.is_unconfirmed = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 123f1931827105ef7c0dd08dc2a2fe66725042c9 and 8c54133a445ce5caa66336ef1727f5140892736d.
📒 Files selected for processing (7)
src/interfaces/wallet.h(1 hunks)src/qt/overviewpage.cpp(1 hunks)src/wallet/interfaces.cpp(2 hunks)src/wallet/receive.cpp(2 hunks)src/wallet/transaction.h(1 hunks)src/wallet/wallet.cpp(3 hunks)src/wallet/wallet.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/qt/overviewpage.cpp
- src/wallet/interfaces.cpp
- src/wallet/wallet.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Image
🔇 Additional comments (4)
src/wallet/transaction.h (1)
292-297: Good addition of the CoinJoinCredits struct.The struct provides a clean encapsulation of CoinJoin-related credit information. The use of meaningful field names and initialization of amounts to zero follows good practices.
src/wallet/receive.cpp (1)
316-326: Well-structured implementation of GetBalanceAnonymized.The method follows good practices:
- Early return if CoinJoin is disabled
- Proper locking with LOCK(cs_wallet)
- Clear variable naming
src/wallet/wallet.cpp (2)
1996-2028: LGTM! Type safety improvements in GetAnonymizedCredit.The changes improve type safety by using a reference instead of a pointer for coinControl parameter, and make the selection check more explicit with HasSelected().
2030-2033: LGTM! Improved credit calculation consolidation.The new method consolidates anonymized and denominated credit calculations into a single call with a structured return type, improving API clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/wallet.h (1)
268-293: LGTM! Consider enhancing documentation.The new structures are well-designed for their purposes. Consider adding:
- Documentation for
COutputEntryfields- Example usage in the class comment for
CMerkleTxstruct COutputEntry { + /** The destination address for this output */ CTxDestination destination; + /** The amount in satoshis */ CAmount amount; + /** The output index in the transaction */ int vout; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8c54133a445ce5caa66336ef1727f5140892736d and e85b8fdb9eecf7a1dff80df0db0ffed5b3a8a2e1.
📒 Files selected for processing (2)
src/wallet/wallet.cpp(3 hunks)src/wallet/wallet.h(2 hunks)
⏰ 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/wallet/wallet.h (4)
247-266: LGTM! Well-structured utility functions for order position handling.The implementation is clean and handles edge cases appropriately:
- Safe handling of missing "n" key in
ReadOrderPos- Skipping writes for negative order positions in
WriteOrderPos
524-529: Initializeis_unconfirmedto avoid potential undefined behavior.The
is_unconfirmedfield inCoinJoinCreditsis not initialized. If it gets used before assignment, it may cause undefined behavior.struct CoinJoinCredits { CAmount m_anonymized{0}; CAmount m_denominated{0}; - bool is_unconfirmed; + bool is_unconfirmed{false}; };
537-539: LGTM! Thread-safety requirements are well-documented.The method is correctly marked with
NO_THREAD_SAFETY_ANALYSISand documents its requirement for the cs_wallet lock.
1093-1094: Verify callers of GetBalance after parameter removal.The removal of the
coinControlparameter fromGetBalancemight affect existing code. Please ensure all callers have been updated.Run the following script to find all callers of
GetBalance:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find all callers of GetBalance to verify they've been updated # after the parameter removal. # Search for GetBalance calls with 4 parameters (old signature) echo "Searching for old GetBalance calls with 4 parameters..." rg -p "GetBalance\([^,]*,[^,]*,[^,]*,[^,]*\)" --type cpp # Search for all GetBalance calls to verify the update echo -e "\nSearching for all GetBalance calls..." rg -p "GetBalance\(" --type cppLength of output: 1742
Review Summary: All GetBalance Callers Updated Successfully
The search results confirm that no old calls using the deprecated 4-parameter signature exist. All current calls pass 0–3 arguments, aligning with the updated signature in CWallet. No further modifications are needed.
src/wallet/wallet.cpp (2)
1996-2028: LGTM! Improved type safety with reference parameter.The function has been updated to use a reference parameter for CCoinControl instead of a pointer, which improves type safety while maintaining the same functionality.
2030-2081: LGTM! Well-structured consolidation of credit calculations.The new function effectively consolidates anonymized and denominated credit calculations into a single method with proper caching, validation, and error handling. This improves code organization and maintainability.
why: - we calculate credits only, so "balance"->"credits" - we calculate unspent credits only, so use word "Available" for func name - "Anonymized" means "fully mixed denominated", so use word "CoinJoin" for struct and func names instead
knst
force-pushed
the
perf-wallet-balance
branch
from
e85b8fd to
df208d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/transaction.h (1)
306-306: Consider adding documentation for the new method.The new method
GetAvailableCoinJoinCreditslacks documentation explaining its purpose and thread safety requirements.Add documentation similar to other methods in the class:
+ /** + * Get available CoinJoin credits including both anonymized and denominated amounts. + * Requires cs_wallet lock. + * @return CoinJoinCredits struct containing credit information + */ CoinJoinCredits GetAvailableCoinJoinCredits() const NO_THREAD_SAFETY_ANALYSIS;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between e85b8fdb9eecf7a1dff80df0db0ffed5b3a8a2e1 and df208d4.
📒 Files selected for processing (2)
src/wallet/transaction.h(1 hunks)src/wallet/wallet.cpp(3 hunks)
⏰ 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 (4)
src/wallet/transaction.h (2)
292-297: LGTM! Well-structured CoinJoinCredits struct.The struct provides a clean encapsulation of CoinJoin-related credit information with clear member initialization.
304-304: Verify impact of parameter type change.The method signature changed from pointer to reference parameter, which is a good practice for type safety. However, we should verify all call sites are updated.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all calls to GetAnonymizedCredit to verify they've been updated rg "GetAnonymizedCredit\s*\(" -A 2Length of output: 672
Verification Outcome: Call sites correctly use the updated reference parameter
- The grep output shows that in both wallet/receive.cpp and wallet.cpp the method GetAnonymizedCredit is invoked or defined with the parameter as a const reference.
- The call in wallet/receive.cpp passes coinControl directly (as a reference), matching the updated signature.
src/wallet/wallet.cpp (2)
1996-2028: LGTM! Improved type safety by using reference instead of pointer.The change from pointer to reference parameter for
coinControleliminates potential null pointer issues and improves type safety while maintaining the same functionality.
2030-2081: LGTM! Well-structured implementation with performance optimizations.The new
GetAvailableCoinJoinCreditsfunction effectively:
- Consolidates credit retrieval into a single method
- Uses caching to improve performance
- Handles both unconfirmed and confirmed transactions correctly
- Includes proper range checking and error handling
|
I have many wallets that are slow to load, however, they are due to mixing, eg +1M addresses, that only get used once. So, I will be happy to test those. However, it sounds like this improvement will help Crowdnode more because they have the opposite scenario, millions of TXes involving the same address. Cheers for this PR ! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/wallet/receive.cpp (3)
316-326: Add documentation for the new method.Please add documentation explaining the purpose of this method, its parameters, and return value. This will help maintainers understand the method's role in anonymized balance calculations.
+/** + * Get the total amount of anonymized funds available in the wallet. + * @param coinControl Parameters for coin selection and other controls + * @return The total amount of anonymized funds + */ CAmount CWallet::GetBalanceAnonymized(const CCoinControl& coinControl) constAlso, consider adding overflow checks when accumulating the anonymized amount:
CAmount anonymized_amount{0}; LOCK(cs_wallet); for (auto pcoin : GetSpendableTXs()) { - anonymized_amount += pcoin->GetAnonymizedCredit(coinControl); + const CAmount credit = pcoin->GetAnonymizedCredit(coinControl); + if (!MoneyRange(credit) || !MoneyRange(anonymized_amount + credit)) { + throw std::runtime_error("CWallet::GetBalanceAnonymized(): value out of range"); + } + anonymized_amount += credit; }
328-365: Consider splitting balance calculation logic.The method is handling multiple types of balances (trusted, untrusted, immature, anonymized, denominated). Consider splitting this into smaller, focused methods for better maintainability.
+CWallet::Balance CWallet::GetStandardBalance(const int min_depth, const bool avoid_reuse, const bool fAddLocked, const std::set<uint256>& trusted_parents, const CWalletTx& wtx) const { + Balance ret; + isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED; + const CAmount tx_credit_mine = wtx.GetAvailableCredit(true, ISMINE_SPENDABLE | reuse_filter); + const CAmount tx_credit_watchonly = wtx.GetAvailableCredit(true, ISMINE_WATCH_ONLY | reuse_filter); + const bool is_trusted = IsTrusted(wtx, trusted_parents); + const int tx_depth = wtx.GetDepthInMainChain(); + + if (is_trusted && ((tx_depth >= min_depth) || (fAddLocked && wtx.IsLockedByInstantSend()))) { + ret.m_mine_trusted = tx_credit_mine; + ret.m_watchonly_trusted = tx_credit_watchonly; + } + if (!is_trusted && tx_depth == 0 && wtx.InMempool()) { + ret.m_mine_untrusted_pending = tx_credit_mine; + ret.m_watchonly_untrusted_pending = tx_credit_watchonly; + } + ret.m_mine_immature = wtx.GetImmatureCredit(); + ret.m_watchonly_immature = wtx.GetImmatureWatchOnlyCredit(); + return ret; +} + +void CWallet::GetCoinJoinBalance(Balance& ret, const CWalletTx& wtx) const { + const auto balance_anonymized = wtx.GetAvailableCoinJoinCredits(); + ret.m_anonymized += balance_anonymized.m_anonymized; + if (balance_anonymized.is_unconfirmed) { + ret.m_denominated_untrusted_pending += balance_anonymized.m_denominated; + } else { + ret.m_denominated_trusted += balance_anonymized.m_denominated; + } +}Also, add overflow checks when accumulating balances:
if (cj_enabled) { const auto balance_anonymized = wtx.GetAvailableCoinJoinCredits(); + if (!MoneyRange(balance_anonymized.m_anonymized) || + !MoneyRange(ret.m_anonymized + balance_anonymized.m_anonymized)) { + throw std::runtime_error("CWallet::GetBalance(): anonymized value out of range"); + } ret.m_anonymized += balance_anonymized.m_anonymized; if (balance_anonymized.is_unconfirmed) { + if (!MoneyRange(balance_anonymized.m_denominated) || + !MoneyRange(ret.m_denominated_untrusted_pending + balance_anonymized.m_denominated)) { + throw std::runtime_error("CWallet::GetBalance(): denominated value out of range"); + } ret.m_denominated_untrusted_pending += balance_anonymized.m_denominated; } else { + if (!MoneyRange(balance_anonymized.m_denominated) || + !MoneyRange(ret.m_denominated_trusted + balance_anonymized.m_denominated)) { + throw std::runtime_error("CWallet::GetBalance(): denominated value out of range"); + } ret.m_denominated_trusted += balance_anonymized.m_denominated; } }
332-332: Consider moving CoinJoin check to a class member.Since CoinJoin status is checked in multiple methods, consider caching it as a class member to avoid repeated calls to
CCoinJoinClientOptions::IsEnabled().class CWallet { + bool m_coinjoin_enabled{false}; + void UpdateCoinJoinStatus() { + m_coinjoin_enabled = CCoinJoinClientOptions::IsEnabled(); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wallet/receive.cpp(2 hunks)
⏰ 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)
| CAmount anonymized_amount{0}; | ||
| LOCK(cs_wallet); | ||
| for (auto pcoin : GetSpendableTXs()) { | ||
| anonymized_amount += pcoin->GetAnonymizedCredit(coinControl); | ||
| } | ||
| return anonymized_amount; |
There was a problem hiding this comment.
could probably use accumulate
| CAmount anonymized_amount{0}; | |
| LOCK(cs_wallet); | |
| for (auto pcoin : GetSpendableTXs()) { | |
| anonymized_amount += pcoin->GetAnonymizedCredit(coinControl); | |
| } | |
| return anonymized_amount; | |
| LOCK(cs_wallet); | |
| return std::accumulate(GetSpendableTXs().begin(), GetSpendableTXs().end(), CAmount{0}, | |
| [&](CAmount sum, const auto& pcoin) { | |
| return sum + pcoin->GetAnonymizedCredit(coinControl); | |
| });``` |
There was a problem hiding this comment.
should not use std::accumulate() without ranges, I think.
GetSpendableTXs() does caclucation inside, it doesn't just return reference to internal structure.
So, begin() and end() can even belong to 2 different data structures in memory.
4 participants
Issue being fixed or feature implemented
Wallets with big amount of transactions are loading too long time.
What was done?
Optimized heavy calculations of balance by Wallet. See commits for details.
It makes Dash-Qt also a bit more responsible because a lot of balance calculations are done in
mainthread to update balances, though "the lagging" is not easy to measure.How Has This Been Tested?
Import pubkey with a lot transaction, for example
yVXDAM73Tg6A44Bm3qduXsMCYxzuqBCT48call
unloadwallet+loadwallet, check time.Loading of wallet is speed up from 24seconds to 23.5 seconds (average by mutiple runs).
here's detailed breakdown to particular functions (see branch with extra logs https://github.com/knst/dash/commits/perf-wallet-balance-with-logs/):
This PR:
Sum-up improvement is slightly higher then real improvement of call
loadwallet(1 second as sum of function vs 0.5 second as total RPC time) due to fact that calculations distributed between 2 threads:mainandqt-rpcconsole.Breaking Changes
N/A
Checklist: