Merge pull request #1 from SamiAhmed7777/cleanup/desloppify

Code cleanup: Documentation and C++11 compliance fixes

.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/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/walletmodel.cpp

@@ -246,7 +246,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)