docs: Document critical TODOs/FIXMEs with context
- Add CLEANUP_NOTES.md documenting cleanup strategy - Add TODO_DOCUMENTATION.md with detailed context for all TODOs - Improve inline comments for thread safety issue in rpcmining.cpp - Clarify potential collision note in walletmodel.cpp - Remove unclear 'DRM' comment, replace with descriptive text No functional changes - documentation only.
This commit is contained in:
Krystie
@@ -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
|
||||
@@ -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.
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
+6
-2
@@ -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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user