diff --git a/CLEANUP_NOTES.md b/CLEANUP_NOTES.md new file mode 100644 index 0000000..ec1c86a --- /dev/null +++ b/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 diff --git a/TODO_DOCUMENTATION.md b/TODO_DOCUMENTATION.md new file mode 100644 index 0000000..eaf0d57 --- /dev/null +++ b/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. diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 13cc767..883b50d 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -246,7 +246,10 @@ WalletModel::SendCoinsReturn WalletModel::sendCoins(const QList 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); } diff --git a/src/rpcmining.cpp b/src/rpcmining.cpp index 583a73d..70059b6 100644 --- a/src/rpcmining.cpp +++ b/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 > 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 vNewBlock; static CReserveKey reservekey(pwalletMain);