GUI freez fix #1686
GUI freez fix #1686
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRemoves a tracker API method, adjusts logging formats, integrates ClientModel block height caching into WalletModel, updates UI Spark gating to use block count, adds Spark wallet balance API, introduces Spark name locking, adds wallet-side private balance caching and TryGetBalances, and makes minor include cleanup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Node as Validation/Chain
participant ClientModel
participant WalletModel
participant UI as Qt Views
participant Wallet as CWallet
participant Spark as CSparkWallet
Node->>ClientModel: BlockTipChanged(new tip)
ClientModel->>ClientModel: cachedNumBlocks = tip.height
ClientModel-->>UI: numBlocksChanged(...)
UI->>WalletModel: pollBalanceChanged()
WalletModel->>WalletModel: currentNumBlocks = clientModel.cachedNumBlocks
WalletModel->>Wallet: TryGetBalances(...)
alt success
Wallet-->>WalletModel: balances (confirmed, unconf, immature)
WalletModel-->>UI: balanceChanged(...)
else lock fail
WalletModel-->>UI: (no update)
end
Note over UI,Spark: Spark enablement uses block count
UI->>UI: spark::IsSparkAllowed(currentNumBlocks)
UI->>Wallet: GetSparkBalance()
alt spark wallet available
Wallet->>Spark: getSparkBalance()
Spark-->>Wallet: {confirmedLike, unconfirmed}
else unavailable
Wallet-->>UI: {0,0}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/qt/sendcoinsdialog.cpp (1)
744-764: src/qt/sendcoinsdialog.cpp – guard null model and reorderIsSparkAllowedcheck
File:src/qt/sendcoinsdialog.cppline 751- auto allowed = (spark::IsSparkAllowed(count) && model->getWallet() && model->getWallet()->sparkWallet); + auto allowed = (model && model->getWallet() && model->getWallet()->sparkWallet && spark::IsSparkAllowed(count));
- Prevents dereferencing a null or uninitialized
modelifnumBlocksChangedfires beforesetModel().- Short-circuits the potentially expensive
IsSparkAllowed(count)call when no wallet is present.- (Optional) Audit other
spark::IsSparkAllowed()uses for consistent gating on the block count.src/qt/clientmodel.cpp (1)
353-366: Only update cachedNumBlocks on block-tip (not header-tip) to avoid overstating block countSetting cachedNumBlocks during header-tip notifications can transiently reflect header height instead of active chain height, leading to incorrect UI gating and metrics. Guard with !fHeader and null-check pIndex.
- clientmodel->cachedNumBlocks = pIndex->nHeight; + if (!fHeader && pIndex) { + clientmodel->cachedNumBlocks = pIndex->nHeight; + }src/wallet/wallet.cpp (1)
2833-2869: Add locking and avoid pwalletMain in member method; tighten cache semantics
- Missing cs_main/cs_wallet locks while reading chainActive and wallet-owned state.
- Using pwalletMain inside CWallet ties this to a global wallet instance and complicates multi-wallet/testing; use the member instead.
- Cache returns only when confirmed>0; consider a validity flag or height-based invalidation to prevent stale returns after spends. At minimum, return cached if either leg is cached and ensure invalidation on wallet mutation.
Apply minimal fixes:
-std::pair<CAmount, CAmount> CWallet::GetPrivateBalance(size_t &confirmed, size_t &unconfirmed) +std::pair<CAmount, CAmount> CWallet::GetPrivateBalance(size_t &confirmed, size_t &unconfirmed) { - if (cachedLelantusBalance.first > 0) - return {cachedLelantusBalance.first, cachedLelantusBalance.second}; + LOCK2(cs_main, cs_wallet); + if (cachedLelantusBalance.first > 0 || cachedLelantusBalance.second > 0) + return {cachedLelantusBalance.first, cachedLelantusBalance.second}; @@ - auto zwallet = pwalletMain->zwallet.get(); + auto zw = this->zwallet.get(); - if(!zwallet) + if(!zw) return balance; - auto lelantusCoins = zwallet->GetTracker().ListLelantusMints(true, false, false); + auto lelantusCoins = zw->GetTracker().ListLelantusMints(true, false, false); @@ - cachedLelantusBalance.first = balance.first; - cachedLelantusBalance.second = balance.second; + cachedLelantusBalance.first = balance.first; + cachedLelantusBalance.second = balance.second; return balance; }Additionally, invalidate the cache on wallet mutation (AddToWallet/MarkDirty/Abandon/MarkConflicted) to avoid staleness. Example (near Line 1293 after MarkDirty), if acceptable:
// Break debit/credit balance caches: wtx.MarkDirty(); + // Invalidate Lelantus balance cache + cachedLelantusBalance = {0, 0};src/sparkname.h (1)
4-9: Missing header for CCriticalSection/LOCK macros.sparkname.h now declares CCriticalSection but doesn’t include its definition. Add sync.h here to keep the header self-contained and avoid ODR/indirect-include hazards.
#include "libspark/spend_transaction.h" +// For CCriticalSection and LOCK/AssertLockHeld macros +#include "sync.h"src/sparkname.cpp (10)
16-30: Don’t call UI notifications while holding cs_spark_name (freeze/deadlock risk).uiInterface.NotifySparkNameRemoved/Added may re-enter code that also needs cs_spark_name, causing UI hangs. Collect events under lock, release it, then notify.
-bool CSparkNameManager::AddBlock(CBlockIndex *pindex, bool fBackupRewrittenEntries) -{ - LOCK(cs_spark_name); - for (const auto &entry : pindex->removedSparkNames) { - sparkNameAddresses.erase(entry.second.sparkAddress); - sparkNames.erase(ToUpper(entry.first)); - uiInterface.NotifySparkNameRemoved(entry.second); - } - for (const auto &entry : pindex->addedSparkNames) { - std::string upperName = ToUpper(entry.first); - if (sparkNames.count(upperName) > 0 && fBackupRewrittenEntries) - pindex->removedSparkNames[upperName] = sparkNames[upperName]; - sparkNames[upperName] = entry.second; - sparkNameAddresses[entry.second.sparkAddress] = upperName; - uiInterface.NotifySparkNameAdded(entry.second); - } - return true; -} +bool CSparkNameManager::AddBlock(CBlockIndex *pindex, bool fBackupRewrittenEntries) +{ + std::vector<CSparkNameBlockIndexData> removedEvents; + std::vector<CSparkNameBlockIndexData> addedEvents; + { + LOCK(cs_spark_name); + for (const auto &entry : pindex->removedSparkNames) { + sparkNameAddresses.erase(entry.second.sparkAddress); + sparkNames.erase(ToUpper(entry.first)); + removedEvents.push_back(entry.second); + } + for (const auto &entry : pindex->addedSparkNames) { + std::string upperName = ToUpper(entry.first); + if (sparkNames.count(upperName) > 0 && fBackupRewrittenEntries) + pindex->removedSparkNames[upperName] = sparkNames[upperName]; + sparkNames[upperName] = entry.second; + sparkNameAddresses[entry.second.sparkAddress] = upperName; + addedEvents.push_back(entry.second); + } + } + for (const auto &ev : removedEvents) uiInterface.NotifySparkNameRemoved(ev); + for (const auto &ev : addedEvents) uiInterface.NotifySparkNameAdded(ev); + return true; +}
37-48: Same: move UI notifications out of cs_spark_name in RemoveBlock.Apply the same event-buffering pattern to avoid UI hangs.
-bool CSparkNameManager::RemoveBlock(CBlockIndex *pindex) -{ - LOCK(cs_spark_name); - for (const auto &entry : pindex->addedSparkNames) { - sparkNames.erase(ToUpper(entry.first)); - sparkNameAddresses.erase(entry.second.sparkAddress); - uiInterface.NotifySparkNameRemoved(entry.second); - } - for (const auto &entry : pindex->removedSparkNames) { - sparkNames[ToUpper(entry.first)] = entry.second; - sparkNameAddresses[entry.second.sparkAddress] = ToUpper(entry.first); - uiInterface.NotifySparkNameAdded(entry.second); - } - return true; -} +bool CSparkNameManager::RemoveBlock(CBlockIndex *pindex) +{ + std::vector<CSparkNameBlockIndexData> removedEvents; + std::vector<CSparkNameBlockIndexData> addedEvents; + { + LOCK(cs_spark_name); + for (const auto &entry : pindex->addedSparkNames) { + sparkNames.erase(ToUpper(entry.first)); + sparkNameAddresses.erase(entry.second.sparkAddress); + removedEvents.push_back(entry.second); + } + for (const auto &entry : pindex->removedSparkNames) { + sparkNames[ToUpper(entry.first)] = entry.second; + sparkNameAddresses[entry.second.sparkAddress] = ToUpper(entry.first); + addedEvents.push_back(entry.second); + } + } + for (const auto &ev : removedEvents) uiInterface.NotifySparkNameRemoved(ev); + for (const auto &ev : addedEvents) uiInterface.NotifySparkNameAdded(ev); + return true; +}
53-60: Data race: GetSparkNames reads shared maps without locking.Writers now hold cs_spark_name; readers must lock too. Add a lock to avoid UB/races.
std::set<std::string> CSparkNameManager::GetSparkNames() { std::set<std::string> result; + LOCK(cs_spark_name); for (const auto &entry : sparkNames) result.insert(entry.second.name); return result; }
62-70: Data race: DumpSparkNameData iterates sparkNames without locking.Guard iteration with cs_spark_name.
std::vector<CSparkNameBlockIndexData> CSparkNameManager::DumpSparkNameData() { std::vector<CSparkNameBlockIndexData> result; result.reserve(sparkNames.size()); + LOCK(cs_spark_name); for (const auto &entry : sparkNames) result.push_back(entry.second); return result; }
72-82: Data race: GetSparkAddress reads sparkNames without locking.Acquire cs_spark_name here as well.
bool CSparkNameManager::GetSparkAddress(const std::string &name, std::string &address) { - auto it = sparkNames.find(ToUpper(name)); + LOCK(cs_spark_name); + auto it = sparkNames.find(ToUpper(name)); if (it != sparkNames.end()) { address = it->second.sparkAddress; return true; } else { return false; } }
84-92: Data race: GetSparkNameBlockHeight reads sparkNames without locking.Lock and keep throws as-is.
uint64_t CSparkNameManager::GetSparkNameBlockHeight(const std::string &name) const { - auto it = sparkNames.find(ToUpper(name)); + LOCK(cs_spark_name); + auto it = sparkNames.find(ToUpper(name)); if (it == sparkNames.end()) throw std::runtime_error("Spark name not found: " + name); size_t height = it->second.sparkNameValidityHeight; return height; }
94-101: Data race: GetSparkNameAdditionalData reads sparkNames without locking.Same fix.
std::string CSparkNameManager::GetSparkNameAdditionalData(const std::string &name) const { - auto it = sparkNames.find(ToUpper(name)); + LOCK(cs_spark_name); + auto it = sparkNames.find(ToUpper(name)); if (it == sparkNames.end()) throw std::runtime_error("Spark name not found: " + name); return it->second.additionalInfo; }
185-197: Partial locking in CheckSparkNameTx leaves races on sparkNames.Access to sparkNames occurs before acquiring cs_spark_name. Lock once around both name and address checks to eliminate TOCTOU/races.
- bool fUpdateExistingRecord = false; - if (sparkNames.count(ToUpper(sparkNameData.name)) > 0) { - if (sparkNames[ToUpper(sparkNameData.name)].sparkAddress != sparkNameData.sparkAddress) - return state.DoS(100, error("CheckSparkNameTx: name already exists")); - fUpdateExistingRecord = true; - } - - { - LOCK(cs_spark_name); - if (!fUpdateExistingRecord && sparkNameAddresses.count(sparkNameData.sparkAddress) > 0) - return state.DoS(100, error("CheckSparkNameTx: spark address is already used for another name")); - } + bool fUpdateExistingRecord = false; + { + LOCK(cs_spark_name); + const std::string upperName = ToUpper(sparkNameData.name); + auto it = sparkNames.find(upperName); + if (it != sparkNames.end()) { + if (it->second.sparkAddress != sparkNameData.sparkAddress) + return state.DoS(100, error("CheckSparkNameTx: name already exists")); + fUpdateExistingRecord = true; + } + if (!fUpdateExistingRecord && sparkNameAddresses.count(sparkNameData.sparkAddress) > 0) + return state.DoS(100, error("CheckSparkNameTx: spark address is already used for another name")); + }
350-364: Move NotifySparkNameAdded outside cs_spark_name.Same rationale as AddBlock: avoid calling UI under lock.
bool CSparkNameManager::AddSparkName(const std::string &name, const std::string &address, uint32_t validityBlocks, const std::string &additionalInfo) { std::string upperName = ToUpper(name); - LOCK(cs_spark_name); - if (sparkNames.count(upperName) > 0 && address != sparkNames[upperName].sparkAddress) - return false; - else if (sparkNameAddresses.count(address) > 0) - return false; - sparkNames[upperName] = CSparkNameBlockIndexData(name, address, validityBlocks, additionalInfo); - sparkNameAddresses[address] = upperName; - uiInterface.NotifySparkNameAdded(sparkNames[upperName]); - return true; + CSparkNameBlockIndexData ev; + { + LOCK(cs_spark_name); + if (sparkNames.count(upperName) > 0 && address != sparkNames[upperName].sparkAddress) + return false; + else if (sparkNameAddresses.count(address) > 0) + return false; + sparkNames[upperName] = CSparkNameBlockIndexData(name, address, validityBlocks, additionalInfo); + sparkNameAddresses[address] = upperName; + ev = sparkNames[upperName]; + } + uiInterface.NotifySparkNameAdded(ev); + return true; }
366-379: Move NotifySparkNameRemoved outside cs_spark_name.Avoid UI under lock.
bool CSparkNameManager::RemoveSparkName(const std::string &name, const std::string &address) { std::string upperName = ToUpper(name); - LOCK(cs_spark_name); - if (sparkNames.count(upperName) == 0 || sparkNameAddresses.count(address) == 0) - return false; - CSparkNameBlockIndexData sparkNameData = sparkNames[upperName]; - sparkNames.erase(upperName); - sparkNameAddresses.erase(address); - uiInterface.NotifySparkNameRemoved(sparkNameData); - return true; + CSparkNameBlockIndexData ev; + { + LOCK(cs_spark_name); + if (sparkNames.count(upperName) == 0 || sparkNameAddresses.count(address) == 0) + return false; + ev = sparkNames[upperName]; + sparkNames.erase(upperName); + sparkNameAddresses.erase(address); + } + uiInterface.NotifySparkNameRemoved(ev); + return true; }
🧹 Nitpick comments (14)
src/lelantus.cpp (1)
565-571: Optional: include group id in the failure log for faster triageAdding the coin group id helps correlate with state and block index.
Apply this diff:
- LogPrintf("CheckLelantusJoinSplitTransaction: serial check failed, serial=%s\n", serials[i].GetHex()); + LogPrintf("CheckLelantusJoinSplitTransaction: serial check failed, serial=%s, group_id=%u\n", + serials[i].GetHex(), ids[i]);src/validation.cpp (1)
647-647: Use precomputed hash and C++ casts in log (optional).Change is fine. For a tiny perf/clarity win, prefer the provided
hashTxandstatic_cast<int>:-LogPrintf("CheckTransaction nHeight=%d, isVerifyDB=%d, isCheckWallet=%d, txHash=%s\n", nHeight, (int)isVerifyDB, (int)isCheckWallet, tx.GetHash().ToString()); +LogPrintf("CheckTransaction nHeight=%d, isVerifyDB=%d, isCheckWallet=%d, txHash=%s\n", + nHeight, static_cast<int>(isVerifyDB), static_cast<int>(isCheckWallet), hashTx.ToString());src/wallet/wallet.h (1)
825-826: Avoid exposing cache state publicly; make it private and document lockingcachedLelantusBalance is public and mutable by anyone. Prefer making it private (or at least protected) and accessing under cs_wallet with helpers to prevent accidental corruption and to clarify locking discipline.
Would you like a follow‑up patch to move this into the private section with accessor methods that assert cs_wallet is held?
src/qt/walletmodel.h (2)
297-298: Initialize _client_model to null to prevent indeterminate readsMinor safety improvement.
- ClientModel *_client_model; + ClientModel *_client_model = nullptr;
137-138: Document threading/ownership for setClientModelClarify expected thread (GUI/main), lifetime, and whether WalletModel stores a raw or observing pointer. Prevents misuse.
src/spark/sparkwallet.h (1)
60-66: Make getSparkBalance() const if it only reads stateIf the method doesn’t mutate internal state, mark it const for consistency with other getters and to enable calls on const CSparkWallet.
- std::pair<CAmount, CAmount> getSparkBalance(); + std::pair<CAmount, CAmount> getSparkBalance() const;src/qt/walletview.cpp (1)
152-161: Also propagate client model in setWalletModel() to cover both call ordersPrevents a window where WalletModel lacks ClientModel if setWalletModel() is called before setClientModel().
void WalletView::setWalletModel(WalletModel *_walletModel) { this->walletModel = _walletModel; + if (clientModel && _walletModel) { + _walletModel->setClientModel(clientModel); + }src/wallet/wallet.cpp (2)
2827-2831: Restore const-correct wrapper to avoid API breakageIf the header still declares a const overload, this non-const definition will break it. Keep const wrapper delegating to the non-const implementation to preserve existing call sites.
Apply:
-std::pair<CAmount, CAmount> CWallet::GetPrivateBalance() +std::pair<CAmount, CAmount> CWallet::GetPrivateBalance() const { - size_t confirmed, unconfirmed; - return GetPrivateBalance(confirmed, unconfirmed); + size_t confirmed{0}, unconfirmed{0}; + // delegate safely from const + return const_cast<CWallet*>(this)->GetPrivateBalance(confirmed, unconfirmed); }
7187-7194: Prefer member access and guard with wallet lockUse the instance’s sparkWallet instead of the global, and take cs_wallet for consistency while accessing member pointers/state.
std::pair<CAmount, CAmount> CWallet::GetSparkBalance() { - auto sparkWallet = pwalletMain->sparkWallet.get(); - - if(!sparkWallet) - return {0, 0}; - - return sparkWallet->getSparkBalance(); + LOCK(cs_wallet); + auto sw = this->sparkWallet.get(); + if(!sw) return {0, 0}; + return sw->getSparkBalance(); }src/qt/walletmodel.cpp (2)
162-167: Seed cached height and force a refresh when wiring ClientModel.Initialize cachedNumBlocks from the client and trigger a balance check so the UI updates promptly once the model is connected.
void WalletModel::setClientModel(ClientModel* client_model) { _client_model = client_model; + if (_client_model) { + cachedNumBlocks = _client_model->cachedNumBlocks; + fForceCheckBalanceChanged = true; + } }If WalletView sets the client model after WalletModel construction (and after the timer starts), this change prevents a stale UI. Please confirm call order.
1360-1369: Lock removal in getSparkNameAddress: confirm manager encapsulates synchronization.Same concern as above: ensure CSparkNameManager::GetSparkAddress is safe without external locks. If not, reintroduce appropriate locking or guard inside the manager.
src/sparkname.h (1)
93-94: New cs_spark_name is appropriate; consider thread-safety annotations.Good addition to serialize access to sparkNames/sparkNameAddresses. To harden, annotate the guarded fields to surface missing locks at compile time (if thread annotations are available).
- std::map<std::string, CSparkNameBlockIndexData> sparkNames; - std::map<std::string, std::string> sparkNameAddresses; + std::map<std::string, CSparkNameBlockIndexData> sparkNames /*GUARDED_BY(cs_spark_name)*/; + std::map<std::string, std::string> sparkNameAddresses /*GUARDED_BY(cs_spark_name)*/;Also document intended lock-order relative to mempool.cs/cs_main in a comment to prevent future inversions.
src/sparkname.cpp (2)
1-11: Include sync.h explicitly for LOCK macro clarity.LOCK is used in this TU; include sync.h directly to avoid relying on transitive includes.
#include "ui_interface.h" +#include "sync.h"
343-348: Nit: safe char handling in ToUpper.toupper has undefined behavior for negative signed char. Cast to unsigned char.
std::string CSparkNameManager::ToUpper(const std::string &str) { std::string result = str; - std::transform(result.begin(), result.end(), result.begin(), ::toupper); + std::transform(result.begin(), result.end(), result.begin(), + [](unsigned char c){ return static_cast<char>(std::toupper(c)); }); return result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/hdmint/tracker.h(0 hunks)src/lelantus.cpp(1 hunks)src/liblelantus/coin.cpp(0 hunks)src/qt/clientmodel.cpp(1 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/sparkmodel.cpp(0 hunks)src/qt/walletmodel.cpp(5 hunks)src/qt/walletmodel.h(3 hunks)src/qt/walletview.cpp(1 hunks)src/spark/sparkwallet.cpp(1 hunks)src/spark/sparkwallet.h(1 hunks)src/sparkname.cpp(9 hunks)src/sparkname.h(1 hunks)src/validation.cpp(2 hunks)src/wallet/wallet.cpp(4 hunks)src/wallet/wallet.h(2 hunks)
💤 Files with no reviewable changes (3)
- src/liblelantus/coin.cpp
- src/qt/sparkmodel.cpp
- src/hdmint/tracker.h
🔇 Additional comments (10)
src/lelantus.cpp (1)
567-567: LGTM: clearer diagnostic by logging serial in hexNo behavior change; safer for debugging and incident triage.
src/validation.cpp (1)
875-876: LGTM: safer printf formatting for booleans.Casting to int removes format-mismatch risk and keeps logs consistent.
src/wallet/wallet.cpp (1)
3257-3275: Non-blocking balances: good; confirm recursive lock behaviorUsing TRY_LOCK prevents GUI stalls. These getters re-lock cs_main/cs_wallet internally; ensure locks are recursive in this codebase to avoid self-deadlock. Otherwise, consider calling underlying helpers that do not lock again.
Expected: CCriticalSection (or equivalent) is recursive; GetBalance()/GetUnconfirmedBalance()/GetImmatureBalance() won’t block when already held by this thread.
src/qt/walletmodel.cpp (3)
6-6: Header include OK.Including clientmodel.h here is appropriate for the new pointer usage.
1095-1099: Good: added wallet lock for Spark balance.LOCK(wallet->cs_wallet) before GetSparkBalance() removes a race on wallet state.
1336-1347: ValidateSparkNameData is internally synchronized
ValidateSparkNameData already acquires cs_spark_name (and mempool.cs) around all shared data; no cs_main lock is needed.src/sparkname.cpp (4)
246-253: LGTM: read under lock in GetSparkNameByAddress.Good synchronization and single traversal of sparkNames via cached key.
384-396: LGTM: validity pruning is locked and returns a snapshot.Good use of erase/iterator pattern within a locked scope and returning a copy for post-processing/notifications by caller.
412-415: LGTM: Reset clears state under lock.Appropriate synchronization.
258-281: Remove lock-splitting suggestion: no inverse lock order found. A repo-wide search shows no code path that takes mempool.cs then cs_spark_name, so the sole nested lock in ValidateSparkNameData follows a consistent ordering and poses no deadlock risk.Likely an incorrect or invalid review comment.
| int currentNumBlocks = 0; | ||
| if (_client_model){ | ||
| currentNumBlocks = _client_model->cachedNumBlocks; | ||
| } else { | ||
| currentNumBlocks = chainActive.Height(); | ||
| } | ||
|
|
||
| if(fForceCheckBalanceChanged || chainActive.Height() != cachedNumBlocks) | ||
| if(fForceCheckBalanceChanged || currentNumBlocks != cachedNumBlocks) |
There was a problem hiding this comment.
Avoid unlocked access to chainActive; defer polling until ClientModel is set.
pollBalanceChanged falls back to chainActive.Height() without cs_main, introducing a data race. Also, the QTimer starts in the constructor before setClientModel runs, so this path will execute. Guard and defer instead of touching chainActive here.
Suggested patch:
- int currentNumBlocks = 0;
- if (_client_model){
- currentNumBlocks = _client_model->cachedNumBlocks;
- } else {
- currentNumBlocks = chainActive.Height();
- }
+ if (!_client_model) {
+ // ClientModel not yet set; defer to avoid cs_main locking and races
+ return;
+ }
+ int currentNumBlocks = _client_model->cachedNumBlocks;Also applies to: 155-155
src/qt/walletmodel.cpp
Outdated
| CAmount newBalance = 0; | ||
| CAmount newUnconfirmedBalance = 0; | ||
| CAmount newImmatureBalance = 0; | ||
| CAmount newWatchOnlyBalance = 0; | ||
| CAmount newWatchUnconfBalance = 0; | ||
| CAmount newWatchImmatureBalance = 0; | ||
| CAmount newAnonymizableBalance = getAnonymizableBalance(); | ||
| CAmount newAnonymizableBalance = 0; | ||
|
|
||
| if (!wallet->TryGetBalances(newBalance, newUnconfirmedBalance, newImmatureBalance)) | ||
| return; |
There was a problem hiding this comment.
Anonymizable balance is never computed (always zero).
newAnonymizableBalance is initialized to 0 and never updated, so the UI will never reflect mintable Spark amount and will stop emitting updates for that field.
Minimal fix:
CAmount newAnonymizableBalance = 0;
if (!wallet->TryGetBalances(newBalance, newUnconfirmedBalance, newImmatureBalance))
return;
+ // This is cheap and uses Spark gating internally.
+ newAnonymizableBalance = getAnonymizableBalance();📝 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.
| CAmount newBalance = 0; | |
| CAmount newUnconfirmedBalance = 0; | |
| CAmount newImmatureBalance = 0; | |
| CAmount newWatchOnlyBalance = 0; | |
| CAmount newWatchUnconfBalance = 0; | |
| CAmount newWatchImmatureBalance = 0; | |
| CAmount newAnonymizableBalance = getAnonymizableBalance(); | |
| CAmount newAnonymizableBalance = 0; | |
| if (!wallet->TryGetBalances(newBalance, newUnconfirmedBalance, newImmatureBalance)) | |
| return; | |
| CAmount newBalance = 0; | |
| CAmount newUnconfirmedBalance = 0; | |
| CAmount newImmatureBalance = 0; | |
| CAmount newWatchOnlyBalance = 0; | |
| CAmount newWatchUnconfBalance = 0; | |
| CAmount newWatchImmatureBalance = 0; | |
| CAmount newAnonymizableBalance = 0; | |
| if (!wallet->TryGetBalances(newBalance, newUnconfirmedBalance, newImmatureBalance)) | |
| return; | |
| // This is cheap and uses Spark gating internally. | |
| newAnonymizableBalance = getAnonymizableBalance(); |
🤖 Prompt for AI Agents
In src/qt/walletmodel.cpp around lines 171 to 180, newAnonymizableBalance is
initialized but never set, so compute and assign it after obtaining the other
balances: either call the wallet API that returns anonymizable/mintable balance
(e.g., use the overload of TryGetBalances that accepts an anonymizable
out-parameter or call the wallet's
GetAnonymizableBalance/GetAnonymizableBalanceByAccount function) and store the
result into newAnonymizableBalance; ensure the call is made only after
confirming TryGetBalances succeeded and before any balance-change
comparisons/notifications so the UI field is updated.
| std::pair<CAmount, CAmount> CSparkWallet::getSparkBalance() { | ||
| std::pair<CAmount, CAmount> result = {0, 0}; | ||
| LOCK(cs_spark_wallet); | ||
| for (auto& it : coinMeta) { | ||
| CSparkMintMeta mint = it.second; | ||
|
|
||
| if (mint.isUsed) | ||
| continue; | ||
|
|
||
| // Not confirmed | ||
| if (mint.nHeight > 1) | ||
| result.first += mint.v; | ||
| else | ||
| result.second += mint.v; | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix maturity classification and avoid double counting at nHeight == 1
getSparkBalance currently treats nHeight > 1 as “not confirmed” (comment) but adds to the first bucket, and other balance methods use mixed thresholds (<1 vs >1). This inconsistency can double count height==1 and produce wrong totals. Align on a single rule: confirmed if nHeight >= 1; unconfirmed if nHeight < 1. Also clarify the in-line comment.
Minimal change in this method:
-// Not confirmed
-if (mint.nHeight > 1)
- result.first += mint.v;
-else
- result.second += mint.v;
+// Confirmed if nHeight >= 1
+if (mint.nHeight >= 1)
+ result.first += mint.v; // mature
+else
+ result.second += mint.v; // unconfirmedFollow-up (outside this hunk) to make the rest consistent:
- getUnconfirmedBalance (Line ~150):
-// Continue if confirmed
-if (mint.nHeight > 1)
+// Continue if confirmed (nHeight >= 1)
+if (mint.nHeight >= 1)
continue;- getAddressUnconfirmedBalance (Line ~195):
-// Not confirmed
-if (mint.nHeight > 1)
+// Skip confirmed (nHeight >= 1)
+if (mint.nHeight >= 1)
continue;- Optionally, rename comments near getAvailableBalance/getAddressAvailableBalance to “Skip unconfirmed (nHeight < 1)” for clarity.
Verification helper:
🏁 Script executed:
#!/bin/bash
# Find all Spark balance checks to ensure thresholds are unified.
rg -n -C2 --type=cpp 'nHeight\s*[<>]=?\s*1' src/spark | sed 's/^/HIT: /'Length of output: 1612
Unify nHeight thresholds across all Spark balance methods
Use nHeight ≥ 1 for confirmed vs nHeight < 1 for unconfirmed everywhere to prevent height == 1 from being mis-classified:
- getSparkBalance: change
to
// Not confirmed if (mint.nHeight > 1) result.first += mint.v; else result.second += mint.v;
// Confirmed (nHeight ≥ 1) if (mint.nHeight >= 1) result.first += mint.v; // mature else result.second += mint.v; // unconfirmed
- getUnconfirmedBalance & getAddressUnconfirmedBalance: replace
with
// Not confirmed if (mint.nHeight < 1) continue;
// Skip confirmed (nHeight ≥ 1) if (mint.nHeight >= 1) continue;
- getAvailableBalance & getAddressAvailableBalance: replace
with
// Continue if confirmed if (mint.nHeight > 1) continue;
// Skip unconfirmed (nHeight < 1) if (mint.nHeight < 1) continue;
- fMatureOnly filter: clarify
to
// Not confirmed if (fMatureOnly && mint.nHeight < 1) continue;
// Skip unconfirmed (nHeight < 1) if (fMatureOnly && mint.nHeight < 1) continue;
🤖 Prompt for AI Agents
src/spark/sparkwallet.cpp lines 103-121: getSparkBalance mis-classifies
height==1 as unconfirmed; change the threshold to treat nHeight >= 1 as
confirmed and nHeight < 1 as unconfirmed (i.e., sum into confirmed when
mint.nHeight >= 1, else into unconfirmed). Also apply the unified rule across
the other Spark balance methods in this file: in getUnconfirmedBalance and
getAddressUnconfirmedBalance skip entries with mint.nHeight >= 1, in
getAvailableBalance and getAddressAvailableBalance skip entries with
mint.nHeight < 1, and for the fMatureOnly check explicitly skip unconfirmed when
mint.nHeight < 1; ensure comments reflect “confirmed (nHeight ≥ 1)” and
“unconfirmed (nHeight < 1)” consistently.
| std::pair<CAmount, CAmount> GetPrivateBalance(); | ||
| std::pair<CAmount, CAmount> GetPrivateBalance(size_t &confirmed, size_t &unconfirmed); | ||
| bool TryGetBalances(CAmount& balance, CAmount& unconfirmedBalance, CAmount& newImmatureBalance) const; | ||
| CAmount GetUnconfirmedBalance() const; |
There was a problem hiding this comment.
🧩 Analysis chain
Constness change of GetPrivateBalance may break callers; confirm all sites updated
GetPrivateBalance(...) became non‑const. Any callers holding a const CWallet* will now fail to compile. Either:
- Re‑add a const overload that forwards to the non‑const implementation, or
- Verify and update all call sites.
Run to list call sites:
🏁 Script executed:
#!/bin/bash
# Find all GetPrivateBalance usages with context
rg -nP -C3 '\bGetPrivateBalance\s*\(' --type=cpp --type=h -g '!**/build/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Locate wallet.h
find . -type f -name 'wallet.h'
# Search for all GetPrivateBalance call sites without type filtering
rg -n 'GetPrivateBalance\s*\(' -g '!**/build/**' --no-ignoreLength of output: 192
🏁 Script executed:
#!/bin/bash
# Search for all GetPrivateBalance usages across src without ignoring any directories
grep -Rnh --exclude-dir=build 'GetPrivateBalance\s*(' srcLength of output: 912
Restore const overloads for GetPrivateBalance
Re-add const-qualified overloads of both GetPrivateBalance() and GetPrivateBalance(size_t&, size_t&) in src/wallet/wallet.h (around lines 940–943) that forward to the existing implementations so any callers holding a const CWallet* (e.g. in walletModel and RPC code) continue to compile successfully.
🤖 Prompt for AI Agents
In src/wallet/wallet.h around lines 940 to 943, callers holding a const CWallet*
fail to compile because const-qualified overloads of GetPrivateBalance() and
GetPrivateBalance(size_t&, size_t&) were removed; restore them by adding const
versions that forward to the existing non-const implementations (call through to
the class's implementations or to the non-const overloads via const_cast if
necessary) so const callers (walletModel, RPC, etc.) continue to compile.
No description provided.