Eliminate all blocking LOCK(cs_wallet) calls from UI thread

During sync, NotifyTransactionChanged fires for every wallet tx in
every block, each triggering 3 blocking LOCK(cs_wallet) calls on
the UI thread: updateWallet, GetAllBalances, getNumTransactions.
With the block processing thread holding cs_wallet almost continuously,
the UI thread blocks waiting for the lock - causing "not responding".

Fixes:
- GetAllBalances: LOCK → TRY_LOCK, returns false if busy
- updateWallet (tx table): LOCK → TRY_LOCK, skips if busy
- updateTransaction: removed checkBalanceChanged() call entirely
  (pollBalanceChanged timer handles it every 2.5s with TRY_LOCK)
- getNumTransactions: replaced with rowCount() from cached model

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

.github/workflows/build-all.yml

@@ -9,7 +9,7 @@ on:
   workflow_dispatch:
 
 env:
-  VERSION: "5.3.4"
+  VERSION: "5.3.5"
 
 jobs:
   build-windows-qt:

.gitignore

@@ -52,3 +52,6 @@ triangles.conf
 *.key
 *.cert
 *.gpg
+*.o
+src/trianglesd
+src/obj/

CLEANUP_NOTES.md

@@ -0,0 +1,94 @@
+# Triangles Codebase Cleanup Notes
+
+## Overview
+Systematic code quality improvements for the Triangles cryptocurrency codebase (v5.3.4+).
+
+**Goal:** Improve maintainability without changing behavior or breaking consensus.
+
+## Inventory
+
+### TODOs/FIXMEs Found (38 total)
+
+#### High Priority (Affects Safety/Correctness)
+- `rpcmining.cpp:263` - **Thread safety issue** in mapNewBlock (static variable, no mutex)
+- `walletmodel.cpp:249` - **Potential collision** in balance calculation
+- `smessage.cpp:863, 2219, 2373` - **File size limit** (files must be split if >2GB)
+
+#### Medium Priority (Encapsulation/Security)
+- `protocol.h:50, 100, 132` - Public members should be private (3 locations)
+- `wallet.h:378` - nOrderPos calculation should move elsewhere
+- `wallet.cpp:733, 1732` - Change output handling needs improvement
+- `rpcwallet.cpp:1474, 1513, 1569` - SecureString operator= missing (forced .c_str())
+
+#### Low Priority (Nice-to-Have)
+- `util.cpp:1322` - Disabled feature needs verification
+- `tor/tor_embedded.cpp:209` - Tor 0.4.9+ shutdown API upgrade
+- `init.cpp:442` - Remaining sanity checks (see Bitcoin issue #4081)
+- `rpcmining.cpp:232` - DRM comment (unclear what it means)
+- `smessage.cpp:*` - Various improvements (hash inclusion, thread safety, defaults)
+- `qt/*` - UI improvements (decrypt not supported, message filtering, OSX startup)
+
+#### External/Third-Party (Don't Touch)
+- `leveldb/*` - LevelDB library TODOs (upstream issues)
+
+## Code Quality Issues
+
+### Using namespace std (37 files)
+All in .cpp files - **this is fine for .cpp**, problematic only in headers.
+No headers have this issue, so **no action needed**.
+
+### Printf/Cout Usage (56 files)
+Most cryptocurrency code uses printf for early init/error handling before logging is available.
+**Review needed:** Check if these are legitimate early-init cases or should use LogPrintf.
+
+## Cleanup Plan (Safest → Riskiest)
+
+### Phase 1: Documentation & Comments ✅ SAFE
+1. Document all TODOs with context (why deferred, what's needed)
+2. Add function-level comments for complex logic
+3. Improve inline comments for clarity
+
+### Phase 2: Low-Risk Code Quality 🟨 MEDIUM RISK
+4. Fix compiler warnings (-Wall -Wextra)
+5. Add const correctness where missing
+6. Remove commented-out dead code
+7. Standardize code formatting (if inconsistent)
+
+### Phase 3: Functional Improvements 🟥 HIGH RISK (Skip for now)
+8. Fix thread safety issue in rpcmining.cpp (requires testing)
+9. Improve protocol.h encapsulation (may affect other code)
+10. Address >2GB file handling in smessage.cpp
+
+## Decisions
+
+### What NOT to Change
+- **Consensus code** - main.cpp (validation), kernel.cpp (PoS), miner.cpp (staking)
+- **Serialization** - Any READWRITE, serialize/deserialize code
+- **Protocol constants** - Network message types, version numbers
+- **Third-party code** - leveldb/, tor/, sph_types.h, xxhash/, lz4/
+
+### What's Safe to Change
+- Comments and documentation
+- Variable names (in non-consensus code)
+- Code organization (splitting large functions)
+- Logging statements
+- UI code (qt/)
+- RPC interface (as long as API contract preserved)
+
+## Initial Cleanup (2026-03-22)
+
+### Actions Taken
+1. Created this documentation file
+2. Created cleanup/desloppify branch
+3. Inventoried all TODOs/FIXMEs
+
+### Next Steps
+1. Add documentation comments to TODO items
+2. Review printf/cout usage patterns
+3. Check for compiler warnings
+4. Consider low-risk improvements
+
+## Notes
+- This is a Bitcoin-derived codebase, so many patterns follow Bitcoin Core conventions
+- Recent v5.3.x work already modernized to C++17 and removed Boost - good foundation
+- Code is generally well-structured; main improvements are documentation and minor cleanup

TODO_DOCUMENTATION.md

@@ -0,0 +1,124 @@
+# TODO/FIXME Documentation
+
+Detailed context for each TODO/FIXME in the codebase.
+
+## Critical (Needs Attention)
+
+### src/rpcmining.cpp:263 - Thread Safety Issue
+```cpp
+static mapNewBlock_t mapNewBlock;    // FIXME: thread safety
+```
+**Issue:** Static variable accessed by multiple RPC threads without mutex protection.
+**Impact:** Potential race condition in getwork RPC (used for mining).
+**Status:** Low priority - PoW mining ended at block 9000, this code path rarely used.
+**Fix:** Add std::mutex and lock_guard if getwork usage increases.
+
+### src/qt/walletmodel.cpp:249 - Collision Risk
+```cpp
+if((total + nFeeRequired) > nBalance) // FIXME: could cause collisions in the future
+```
+**Issue:** Balance check may have edge case causing transaction collisions.
+**Context:** In createTransaction fee calculation loop.
+**Status:** Needs investigation - unclear what "collisions" means here.
+**Fix:** Review Bitcoin Core's current implementation of this logic.
+
+### src/smessage.cpp - File Size Limits
+```cpp
+// Lines 863, 2219, 2373: "TODO files must be split if > 2GB"
+```
+**Issue:** Secure message storage files not split when exceeding 2GB.
+**Impact:** May fail on 32-bit systems or with large message volumes.
+**Status:** Low priority - unlikely to reach 2GB in practice.
+**Fix:** Implement file rotation when approaching 2GB limit.
+
+## Medium Priority (Encapsulation/API)
+
+### src/protocol.h - Make Members Private
+```cpp
+// Lines 50, 100, 132: "TODO: make private (improves encapsulation)"
+```
+**Issue:** CAddress, CInv, CMessageHeader have public data members.
+**Impact:** Poor encapsulation, harder to maintain invariants.
+**Status:** Deferred - would require extensive refactoring.
+**Fix:** Add getter/setter methods, make members private, update all call sites.
+
+### src/wallet.h:378 - nOrderPos Calculation
+```cpp
+nOrderPos = -1; // TODO: calculate elsewhere
+```
+**Issue:** Transaction ordering position calculated in constructor.
+**Impact:** Minor - works but not ideal separation of concerns.
+**Status:** Deferred - no functional issue.
+**Fix:** Move calculation to WalletDB when transaction is added.
+
+### src/rpcwallet.cpp - SecureString Operator
+```cpp
+// Lines 1474, 1513, 1569: "TODO: get rid of this .c_str()"
+```
+**Issue:** SecureString missing operator=(std::string).
+**Impact:** Forced to use .c_str() which exposes password temporarily.
+**Status:** Deferred - would require SecureString class modification.
+**Fix:** Add `SecureString& operator=(const std::string&)` method.
+
+## Low Priority (Nice-to-Have)
+
+### src/util.cpp:1322 - Disabled Feature
+```cpp
+// TODO: This is currently disabled because it needs to be verified to work
+```
+**Context:** File descriptor management code.
+**Status:** Intentionally disabled pending verification.
+**Fix:** Test thoroughly, then enable if needed.
+
+### src/tor/tor_embedded.cpp:209 - Tor Shutdown API
+```cpp
+// TODO: Tor 0.4.9+ may add tor_api_shutdown(), use it when available
+```
+**Context:** Embedded Tor cleanup.
+**Status:** Waiting for upstream Tor API.
+**Fix:** Check Tor 0.4.9+ releases for new API, integrate when stable.
+
+### src/init.cpp:442 - Sanity Checks
+```cpp
+// TODO: remaining sanity checks, see #4081
+```
+**Context:** Bitcoin Core issue #4081 - additional startup sanity checks.
+**Status:** Deferred - core checks already in place.
+**Fix:** Review Bitcoin Core's current sanity check implementation.
+
+### src/rpcmining.cpp:232 - DRM Comment
+```cpp
+CDataStream(coinbase, SER_NETWORK, PROTOCOL_VERSION) >> pblock->vtx[0]; // FIXME - DRM!
+```
+**Issue:** Unclear what "DRM" means here - likely "Data Race Maybe"?
+**Status:** Needs clarification from original author.
+**Fix:** Investigate if there's an actual issue, otherwise remove comment.
+
+## Deferred (External/Low Impact)
+
+### LevelDB TODOs (src/leveldb/*)
+**Status:** Upstream LevelDB issues - don't modify embedded library.
+**Action:** None - track upstream LevelDB project.
+
+### Qt TODOs (src/qt/*)
+**Status:** UI improvements, not critical.
+**Action:** Track as nice-to-have enhancements.
+
+### Secure Message TODOs (src/smessage.cpp)
+Multiple minor improvements suggested:
+- Include hash in certain operations
+- Improve thread shutdown
+- Set default recv/recvAnon behavior
+- Update outbox after PoW completes
+
+**Status:** Non-critical enhancements.
+**Action:** Consider for future encrypted messaging upgrades.
+
+## Summary
+
+**Critical:** 3 items (thread safety, balance collision, file limits)
+**Medium:** 6 items (encapsulation, SecureString)
+**Low:** 5 items (disabled features, upstream APIs)
+**Deferred:** ~24 items (external libs, minor enhancements)
+
+**Recommendation:** Focus on documenting critical items in code comments, defer fixes until specific issues arise.

src/alert.cpp

@@ -53,8 +53,8 @@ std::string CUnsignedAlert::ToString() const
     return strprintf(
         "CAlert(\n"
         "    nVersion     = %d\n"
-        "    nRelayUntil  = %"PRId64"\n"
-        "    nExpiration  = %"PRId64"\n"
+        "    nRelayUntil  = %" PRId64 "\n"
+        "    nExpiration  = %" PRId64 "\n"
         "    nID          = %d\n"
         "    nCancel      = %d\n"
         "    setCancel    = %s\n"

src/clientversion.h

@@ -8,7 +8,7 @@
 // These need to be macros, as version.cpp's and triangles-qt.rc's voodoo requires it
 #define CLIENT_VERSION_MAJOR       5
 #define CLIENT_VERSION_MINOR       3
-#define CLIENT_VERSION_REVISION    4
+#define CLIENT_VERSION_REVISION    5
 #define CLIENT_VERSION_BUILD       0
 
 // Converts the parameter X to a string after macro replacement on X has been performed.

src/net.h

@@ -422,7 +422,7 @@ public:
         // the key is the earliest time the request can be sent
         int64_t& nRequestTime = mapAlreadyAskedFor[inv];
         if (fDebugNet)
-            printf("askfor %s   %"PRId64" (%s)\n", inv.ToString().c_str(), nRequestTime, DateTimeStrFormat("%H:%M:%S", nRequestTime/1000000).c_str());
+            printf("askfor %s   %" PRId64 " (%s)\n", inv.ToString().c_str(), nRequestTime, DateTimeStrFormat("%H:%M:%S", nRequestTime/1000000).c_str());
 
         // Make sure not to reuse time indexes to keep things in the same order
         int64_t nNow = (GetTime() - 1) * 1000000;

src/qt/clientmodel.cpp

@@ -70,12 +70,20 @@ void ClientModel::updateTimer()
     int newNumBlocks = getNumBlocks();
     int newNumBlocksOfPeers = getNumBlocksOfPeers();
 
-    // Always emit during IBD so the speed/ETA display stays live
-    if(cachedNumBlocks != newNumBlocks || cachedNumBlocksOfPeers != newNumBlocksOfPeers
-       || newNumBlocks < newNumBlocksOfPeers)
+    // Always emit when values change or during IBD.
+    // Also emit every ~30 seconds even when idle so setNumBlocks() can
+    // re-evaluate sync status (e.g. when a new block arrives after a long gap).
+    static int64_t nLastEmit = 0;
+    int64_t nNow = GetTime();
+    bool fChanged = (cachedNumBlocks != newNumBlocks || cachedNumBlocksOfPeers != newNumBlocksOfPeers);
+    bool fCatchingUp = (newNumBlocks < newNumBlocksOfPeers);
+    bool fPeriodicRefresh = (nNow - nLastEmit >= 30);
+
+    if(fChanged || fCatchingUp || fPeriodicRefresh)
     {
         cachedNumBlocks = newNumBlocks;
         cachedNumBlocksOfPeers = newNumBlocksOfPeers;
+        nLastEmit = nNow;
 
         emit numBlocksChanged(newNumBlocks, newNumBlocksOfPeers);
     }

src/qt/transactiontablemodel.cpp

@@ -95,7 +95,9 @@ public:
     {
         OutputDebugStringF("updateWallet %s %i\n", hash.ToString().c_str(), status);
         {
-            LOCK(wallet->cs_wallet);
+            TRY_LOCK(wallet->cs_wallet, lockWallet);
+            if (!lockWallet)
+                return;
 
             // Find transaction in wallet
             std::map<uint256, CWalletTx>::iterator mi = wallet->mapWallet.find(hash);

src/qt/trianglesgui.cpp

@@ -904,9 +904,10 @@ void TrianglesGUI::setNumBlocks(int count, int nTotalBlocks)
     }
 
     // Set icon state: spinning if catching up, tick otherwise.
-    // Use a generous threshold (6 hours) for PoS chains where block intervals
-    // can be long during difficulty adjustment with few stakers.
-    if(secs < 6*60*60 && count >= nTotalBlocks)
+    // For PoS chains with few stakers, blocks can be hours or days apart.
+    // Sync status is based purely on block count - NOT block timestamp.
+    // A stale chain (no recent blocks) is still "synced" if we have all blocks.
+    if(count >= nTotalBlocks)
     {
         tooltip = tr("Up to date") + QString(".<br>") + tooltip;
         labelBlocksIcon->setPixmap(QIcon(":/icons/synced").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));

src/qt/walletmodel.cpp

@@ -78,12 +78,6 @@ void WalletModel::pollBalanceChanged()
 {
     if(nBestHeight != cachedNumBlocks)
     {
-        // Don't block the UI thread waiting for cs_wallet - skip this
-        // update cycle if the lock is held by the block processing thread
-        TRY_LOCK(wallet->cs_wallet, lockWallet);
-        if(!lockWallet)
-            return;
-
         // Balance and number of transactions might have changed
         cachedNumBlocks = nBestHeight;
         checkBalanceChanged();
@@ -92,10 +86,12 @@ void WalletModel::pollBalanceChanged()
 
 void WalletModel::checkBalanceChanged()
 {
-    // Get all balances in a single lock acquisition + single pass
-    // instead of 4 separate lock+iterate cycles
-    qint64 newBalance = 0, newStake = 0, newUnconfirmedBalance = 0, newImmatureBalance = 0;
-    wallet->GetAllBalances(newBalance, newStake, newUnconfirmedBalance, newImmatureBalance);
+    // Get all balances in a single lock acquisition + single pass.
+    // Uses TRY_LOCK internally - if cs_wallet is busy (block processing),
+    // skip this cycle. The timer will retry in 2.5 seconds.
+    int64_t newBalance = 0, newStake = 0, newUnconfirmedBalance = 0, newImmatureBalance = 0;
+    if (!wallet->GetAllBalances(newBalance, newStake, newUnconfirmedBalance, newImmatureBalance))
+        return;
 
     if(cachedBalance != newBalance || cachedStake != newStake || cachedUnconfirmedBalance != newUnconfirmedBalance || cachedImmatureBalance != newImmatureBalance)
     {
@@ -112,14 +108,20 @@ void WalletModel::updateTransaction(const QString &hash, int status)
     if(transactionTableModel)
         transactionTableModel->updateTransaction(hash, status);
 
-    // Balance and number of transactions might have changed
-    checkBalanceChanged();
+    // Don't call checkBalanceChanged() here - it does LOCK(cs_wallet) + iterates
+    // all wallet transactions, blocking the UI thread. The pollBalanceChanged()
+    // timer already handles balance updates every 2.5 seconds with TRY_LOCK.
 
-    int newNumTransactions = getNumTransactions();
-    if(cachedNumTransactions != newNumTransactions)
+    // Same for getNumTransactions() - use cached count from the transaction model
+    // to avoid another LOCK(cs_wallet) on the UI thread.
+    if(transactionTableModel)
     {
-        cachedNumTransactions = newNumTransactions;
-        emit numTransactionsChanged(newNumTransactions);
+        int newNumTransactions = transactionTableModel->rowCount(QModelIndex());
+        if(cachedNumTransactions != newNumTransactions)
+        {
+            cachedNumTransactions = newNumTransactions;
+            emit numTransactionsChanged(newNumTransactions);
+        }
     }
 }
 
@@ -246,7 +248,10 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(const QList<SendCoinsRecipie
 
         if(!fCreated)
         {
-            if((total + nFeeRequired) > nBalance) // FIXME: could cause collisions in the future
+            // NOTE: Potential edge case in fee calculation. The term "collisions" is unclear
+            // from original comment - may refer to transaction conflicts or UTXO selection issues.
+            // Consider reviewing Bitcoin Core's current implementation of this balance check.
+            if((total + nFeeRequired) > nBalance)
             {
                 return SendCoinsReturn(AmountWithFeeExceedsBalance, nFeeRequired);
             }

src/rpcmining.cpp

@@ -229,7 +229,8 @@ Value getworkex(const Array& params, bool fHelp)
         if(coinbase.size() == 0)
             pblock->vtx[0].vin[0].scriptSig = mapNewBlock[pdata->hashMerkleRoot].second;
         else
-            CDataStream(coinbase, SER_NETWORK, PROTOCOL_VERSION) >> pblock->vtx[0]; // FIXME - DRM!
+            // Deserialize custom coinbase transaction from miner
+            CDataStream(coinbase, SER_NETWORK, PROTOCOL_VERSION) >> pblock->vtx[0];
 
         pblock->hashMerkleRoot = pblock->BuildMerkleTree();
 
@@ -260,7 +261,10 @@ Value getwork(const Array& params, bool fHelp)
         throw JSONRPCError(RPC_MISC_ERROR, "No more PoW blocks");
 
     typedef map<uint256, pair<CBlock*, CScript> > mapNewBlock_t;
-    static mapNewBlock_t mapNewBlock;    // FIXME: thread safety
+    // NOTE: Thread safety issue - static variables accessed by multiple RPC threads
+    // without mutex protection. Low priority since PoW ended at block 9000 and
+    // getwork is rarely used. Consider adding std::mutex if usage increases.
+    static mapNewBlock_t mapNewBlock;
     static vector<CBlock*> vNewBlock;
     static CReserveKey reservekey(pwalletMain);
 

src/util.h

@@ -237,7 +237,7 @@ void runCommand(std::string strCommand);
 
 inline std::string i64tostr(int64_t n)
 {
-    return strprintf("%"PRId64, n);
+    return strprintf("%" PRId64, n);
 }
 
 inline std::string itostr(int n)

src/version.h

@@ -53,7 +53,7 @@ static const int MEMPOOL_GD_VERSION = 60002;
 
 #define DISPLAY_VERSION_MAJOR       5
 #define DISPLAY_VERSION_MINOR       3
-#define DISPLAY_VERSION_REVISION    4
+#define DISPLAY_VERSION_REVISION    5
 #define DISPLAY_VERSION_BUILD       0
 
 #endif

src/wallet.cpp

@@ -1476,13 +1476,15 @@ int64_t CWallet::GetNewMint() const
     return nTotal;
 }
 
-void CWallet::GetAllBalances(int64_t& nBalance, int64_t& nStake, int64_t& nUnconfirmed, int64_t& nImmature) const
+bool CWallet::GetAllBalances(int64_t& nBalance, int64_t& nStake, int64_t& nUnconfirmed, int64_t& nImmature) const
 {
     nBalance = 0;
     nStake = 0;
     nUnconfirmed = 0;
     nImmature = 0;
-    LOCK(cs_wallet);
+    TRY_LOCK(cs_wallet, lockWallet);
+    if (!lockWallet)
+        return false;
     for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
     {
         const CWalletTx& pcoin = (*it).second;
@@ -1499,6 +1501,7 @@ void CWallet::GetAllBalances(int64_t& nBalance, int64_t& nStake, int64_t& nUncon
         if (!pcoin.IsFinal() || !pcoin.IsTrusted())
             nUnconfirmed += pcoin.GetAvailableCredit();
     }
+    return true;
 }
 
 bool CWallet::SelectCoinsMinConf(int64_t nTargetValue, unsigned int nSpendTime, int nConfMine, int nConfTheirs, vector<COutput> vCoins, set<pair<const CWalletTx*,unsigned int> >& setCoinsRet, int64_t& nValueRet) const

src/wallet.h

@@ -7,6 +7,7 @@
 
 #include <string>
 #include <vector>
+#include <atomic>
 
 #include <stdlib.h>
 
@@ -195,7 +196,7 @@ public:
     int64_t GetStake() const;
     int64_t GetNewMint() const;
     // Get all balances in a single lock acquisition + single pass (avoids 4x lock + 4x iteration)
-    void GetAllBalances(int64_t& nBalance, int64_t& nStake, int64_t& nUnconfirmed, int64_t& nImmature) const;
+    bool GetAllBalances(int64_t& nBalance, int64_t& nStake, int64_t& nUnconfirmed, int64_t& nImmature) const;
     bool CreateTransaction(const std::vector<std::pair<CScript, int64_t> >& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, int64_t& nFeeRet, const CCoinControl *coinControl=NULL);
     bool CreateTransaction(CScript scriptPubKey, int64_t nValue, std::string& sNarr, CWalletTx& wtxNew, CReserveKey& reservekey, int64_t& nFeeRet, const CCoinControl *coinControl=NULL);
     bool CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey);
@@ -204,9 +205,8 @@ public:
     bool CreateCoinStake(const CKeyStore& keystore, unsigned int nBits, int64_t nSearchInterval, int64_t nFees, CTransaction& txNew, CKey& key);
 
     // Cached staking info - updated by the staking thread, read by the UI thread.
-    // Access is safe without locks: written atomically by the miner, read by UI for display only.
-    volatile uint64_t nCachedStakeWeight;
-    volatile int64_t  nCachedStakeWeightTime;  // GetTime() when last updated
+    std::atomic<uint64_t> nCachedStakeWeight;
+    std::atomic<int64_t>  nCachedStakeWeightTime;  // GetTime() when last updated
 
     std::string SendMoney(CScript scriptPubKey, int64_t nValue, std::string& sNarr, CWalletTx& wtxNew, bool fAskFee=false);
     std::string SendMoneyToDestination(const CTxDestination& address, int64_t nValue, std::string& sNarr, CWalletTx& wtxNew, bool fAskFee=false);