feat: implement Android-style logging system #603
Conversation
- Remove all legacy DSLog/DSLogPrivate statements from codebase (70+ files) - Add new Android-style logging macros: DSLogInfo, DSLogDebug, DSLogWarn, DSLogError, DSLogVerbose - Implement DSCurrentThreadName() helper for thread identification - Add DSAndroidStyleLogFormatter for HH:mm:ss timestamp format matching Android/DashJ New logging coverage: - Transaction broadcasting and relay tracking - Transaction reception from peers (with/without blocks) - InstantSend lock reception and verification - ChainLock reception and verification - Peer connection lifecycle (connect, disconnect, version handshake) - Chain sync start/stop/completion Log format: "HH:MM:SS [thread-name] ClassName - message" Log compaction: 5MB max file size, 24hr rolling, 10 files max, gzip compression Also adds CLAUDE.md documentation files for AI agent context. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdded a structured logger and sync-speed calculator; removed or consolidated many legacy DSLog calls; introduced chain/masternode/coinjoin factory/util APIs, Core Data helpers, masternode/entity utilities, and new documentation. Most edits are logging changes or new monitoring/util APIs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
DashSync/shared/Models/Managers/Chain Managers/DSMasternodeManager.m (1)
809-821: Add parameter validation and documentation for the new public API.This new public class method lacks input validation and documentation:
- Parameter validation: No checks for nil parameters (masternodeList, chain, modifiedMasternodes, context). If callers pass nil values, this could lead to crashes in DSMasternodeListStore.
- Documentation: Public APIs should include header comments explaining parameters, behavior, and thread safety requirements.
- Completion safety: Verify whether DSMasternodeListStore handles nil completion blocks safely, or add a nil check before delegation.
✅ Suggested improvements
+ (void)saveMasternodeList:(DSMasternodeList *)masternodeList toChain:(DSChain *)chain havingModifiedMasternodes:(NSDictionary *)modifiedMasternodes createUnknownBlocks:(BOOL)createUnknownBlocks inContext:(NSManagedObjectContext *)context completion:(void (^)(NSError *error))completion { + NSParameterAssert(masternodeList); + NSParameterAssert(chain); + NSParameterAssert(modifiedMasternodes); + NSParameterAssert(context); + NSParameterAssert(completion); + [DSMasternodeListStore saveMasternodeList:masternodeList toChain:chain havingModifiedMasternodes:modifiedMasternodes createUnknownBlocks:createUnknownBlocks inContext:context completion:completion]; }Additionally, add documentation in the header file (DSMasternodeManager.h):
/** * Saves a masternode list to persistent storage. * * @param masternodeList The masternode list to save (must not be nil) * @param chain The blockchain instance (must not be nil) * @param modifiedMasternodes Dictionary of modified masternodes (must not be nil) * @param createUnknownBlocks Whether to create placeholder blocks for unknown block hashes * @param context The Core Data managed object context for persistence (must not be nil) * @param completion Completion handler called with any error (must not be nil) */ + (void)saveMasternodeList:(DSMasternodeList *)masternodeList toChain:(DSChain *)chain havingModifiedMasternodes:(NSDictionary *)modifiedMasternodes createUnknownBlocks:(BOOL)createUnknownBlocks inContext:(NSManagedObjectContext *)context completion:(void (^)(NSError *error))completion;DashSync/shared/Models/Managers/Service Managers/DSEventManager.m (1)
294-301: Add error handling for NSOutputStream and NSJSONSerialization operations.The code does not verify that
[os open]succeeds at line 295 or check theerrorparameter fromNSJSONSerializationat line 296. If either operation fails, the buffer is cleared at line 300 without any indication that data was lost. The directory creation earlier in the method (lines 278-283) demonstrates proper error handling with early return on failure—the file write operations should follow the same pattern.At minimum, check both the stream open result and the
errorparameter, and log or handle write failures appropriately rather than silently discarding event data.DashSync/shared/Models/Persistence/DSDataController.m (1)
98-111: Check the return value ofremoveItemAtURL:error:before proceeding.The removal operation at line 104 does not check if it succeeded; the BOOL return value is ignored, and the operation proceeds unconditionally to
loadPersistentStoresWithCompletionHandler. Even if the file removal fails, the subsequent store load may proceed with a corrupted or stale store.At minimum, capture the BOOL result and check it. Additionally, per Apple's Core Data recovery guidance, direct file deletion with NSFileManager is discouraged; prefer Core Data APIs like
migratePersistentStoreor read-only recovery to preserve and export user data before destructive steps.
🤖 Fix all issues with AI agents
In @CLAUDE.md:
- Around line 38-49: Add a language identifier to the fenced code block that
contains the DashSync directory tree (the block starting with "DashSync/") so
markdown linters recognize it as plain text; update the opening triple-backticks
to include a language token such as "text" or "plaintext" (e.g., change ``` to
```text) while keeping the directory tree content unchanged.
In @DashSync/shared/Models/CLAUDE.md:
- Around line 35-45: The three Markdown fenced code blocks showing the model
hierarchy, manager hierarchy, and transaction inheritance should include a
language identifier (e.g., text or plaintext) for consistency; update the block
that begins with "DSChain (root object)" (references to DSChain, DSWallet,
DSAccount, DSDerivationPath), the block that begins with "DSChainsManager
(singleton)" (references to DSChainsManager, DSChainManager), and the block that
begins with "DSTransaction (base)" (references to DSTransaction,
DSCoinbaseTransaction) by changing their opening fences from ``` to ```text (or
```plaintext).
In @DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m:
- Around line 656-661: The loop calls script_pubkey_for_address twice per
iteration causing a leak; call script_pubkey_for_address([address UTF8String],
wrapper.chain.chainType) once, store its returned ByteArray in a temporary
variable (e.g., result), then set byteArray->ptr = result.ptr and byteArray->len
= result.len and assign keys->items[i] = byteArray; ensure you don't overwrite
or drop the allocated result without transferring ownership (or free the
temporary if ownership is copied) so only a single allocation happens per
address.
- Around line 646-650: The boolean check in getCoinJoinKeys is inverted: when
the local variable used is true it incorrectly calls wrapper.manager
getIssuedReceiveAddresses; swap the branches so that when used is true you call
wrapper.manager getUsedReceiveAddresses and when false call wrapper.manager
getIssuedReceiveAddresses to ensure addresses gets the correct subset.
In @DashSync/shared/Models/Persistence/Migration/DSCoreDataMigrator.m:
- Around line 110-115: The backup-exclusion call using [url
setResourceValue:forKey:NSURLIsExcludedFromBackupKey error:&error] may fail but
is only asserted later; check the error immediately after that call (inspect the
local error variable) and handle failure before proceeding to the subsequent
WAL/SHM file copy operations, e.g., move the NSAssert (or return/handle)
directly after setResourceValue:forKey:error: so the code copying WAL/SHM files
(the loop operating on WAL/SHM filenames) runs only if the exclusion succeeded.
In @Example/CLAUDE.md:
- Around line 22-31: The code block showing the directory tree in
Example/CLAUDE.md lacks a language identifier, which trips markdown linters;
update the fenced block opening from ``` to ```text (or ```plaintext) so the
directory structure is declared as plain text (i.e., change the code fence
around the Example/ directory listing to include "text").
🧹 Nitpick comments (2)
DashSync/shared/Models/Managers/CLAUDE.md (1)
7-11: Add language identifier to fenced code block.The fenced code block at line 7 should specify a language identifier for proper rendering and linting compliance.
📝 Proposed fix
-``` +```text Managers/ ├── Chain Managers/ # Blockchain operation coordinators └── Service Managers/ # External service coordinators</details> Based on static analysis hints. </blockquote></details> <details> <summary>DashSync/shared/Libraries/DSLogger.m (1)</summary><blockquote> `34-42`: **Fragile parsing of NSThread description for thread number.** The thread description parsing relies on the internal format of `[NSThread description]` (e.g., `"number = X,"`), which is not part of any public API contract and could change between OS versions. Consider using `pthread_self()` or `[NSThread threadDictionary]` for a more stable identifier. <details> <summary>♻️ More robust alternative</summary> ```diff // Use thread number as fallback - NSString *description = thread.description; - NSRange numRange = [description rangeOfString:@"number = "]; - if (numRange.location != NSNotFound) { - NSUInteger start = numRange.location + numRange.length; - NSRange endRange = [description rangeOfString:@"," options:0 range:NSMakeRange(start, description.length - start)]; - if (endRange.location != NSNotFound) { - return [NSString stringWithFormat:@"Thread-%@", [description substringWithRange:NSMakeRange(start, endRange.location - start)]]; - } - } - return @"Thread"; + // Use pthread identifier as a stable fallback + uint64_t threadId; + pthread_threadid_np(NULL, &threadId); + return [NSString stringWithFormat:@"Thread-%llu", threadId];Note: You'll need to
#import <pthread.h>at the top of the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
CLAUDE.mdDashSync/shared/Categories/NSData/NSData+Dash.mDashSync/shared/Categories/NSManagedObject+Sugar.mDashSync/shared/Categories/NSManagedObjectContext+DSSugar.mDashSync/shared/DashSync.mDashSync/shared/Libraries/AdvancedOperations/Operations/DSOperation.mDashSync/shared/Libraries/DSLogger.hDashSync/shared/Libraries/DSLogger.mDashSync/shared/Libraries/DSMerkleTree.mDashSync/shared/Models/CLAUDE.mdDashSync/shared/Models/Chain/DSBlock.mDashSync/shared/Models/Chain/DSChain.mDashSync/shared/Models/Chain/DSChainLock.mDashSync/shared/Models/Chain/DSFullBlock.mDashSync/shared/Models/Chain/DSMerkleBlock.mDashSync/shared/Models/CoinJoin/DSCoinJoinManager.mDashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.mDashSync/shared/Models/CoinJoin/Utils/DSMasternodeGroup.mDashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.mDashSync/shared/Models/DAPI/NSPredicate+CBORData.mDashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.mDashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.mDashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.mDashSync/shared/Models/Derivation Paths/DSDerivationPath.mDashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.mDashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.mDashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.mDashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.mDashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.mDashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.mDashSync/shared/Models/Entities/DSTxOutputEntity+CoreDataClass.mDashSync/shared/Models/Governance/DSGovernanceObject.mDashSync/shared/Models/Identity/DSBlockchainIdentity.mDashSync/shared/Models/Managers/CLAUDE.mdDashSync/shared/Models/Managers/Chain Managers/DSChainManager.mDashSync/shared/Models/Managers/Chain Managers/DSChainsManager.mDashSync/shared/Models/Managers/Chain Managers/DSGovernanceSyncManager.mDashSync/shared/Models/Managers/Chain Managers/DSIdentitiesManager.mDashSync/shared/Models/Managers/Chain Managers/DSKeyManager.mDashSync/shared/Models/Managers/Chain Managers/DSMasternodeManager+Mndiff.mDashSync/shared/Models/Managers/Chain Managers/DSMasternodeManager.mDashSync/shared/Models/Managers/Chain Managers/DSPeerManager.mDashSync/shared/Models/Managers/Chain Managers/DSSporkManager.mDashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.mDashSync/shared/Models/Managers/Service Managers/Auth/DSAuthenticationManager.mDashSync/shared/Models/Managers/Service Managers/DSEventManager.mDashSync/shared/Models/Managers/Service Managers/DSInsightManager.mDashSync/shared/Models/Managers/Service Managers/DSShapeshiftManager.mDashSync/shared/Models/Managers/Service Managers/DSVersionManager.mDashSync/shared/Models/Masternode/DSMasternodeList.mDashSync/shared/Models/Masternode/DSMasternodeListDiffService.mDashSync/shared/Models/Masternode/DSMasternodeListService.mDashSync/shared/Models/Masternode/DSMasternodeListStore.mDashSync/shared/Models/Masternode/DSMasternodeProcessorContext.mDashSync/shared/Models/Masternode/DSMnDiffProcessingResult.mDashSync/shared/Models/Masternode/DSQRInfoProcessingResult.mDashSync/shared/Models/Masternode/DSQuorumEntry.mDashSync/shared/Models/Masternode/DSQuorumRotationService.mDashSync/shared/Models/Masternode/DSSimplifiedMasternodeEntry.mDashSync/shared/Models/Network/DSPeer.mDashSync/shared/Models/Payment/DSPaymentProtocol.mDashSync/shared/Models/Payment/DSPaymentRequest.mDashSync/shared/Models/Persistence/DSDataController.mDashSync/shared/Models/Persistence/Migration/DSCoreDataMigrator.mDashSync/shared/Models/Persistence/Migration/Policies/DSDeleteEntityMigrationPolicy.mDashSync/shared/Models/Transactions/Base/DSInstantSendTransactionLock.mDashSync/shared/Models/Transactions/Base/DSTransaction.mDashSync/shared/Models/Transactions/Provider/DSProviderRegistrationTransaction.mDashSync/shared/Models/Transactions/Provider/DSProviderUpdateRegistrarTransaction.mDashSync/shared/Models/Wallet/DSAccount.mDashSync/shared/Models/Wallet/DSBIP39Mnemonic.mDashSync/shared/Models/Wallet/DSSpecialTransactionsWalletHolder.mDashSync/shared/Models/Wallet/DSWallet.mExample/CLAUDE.md
💤 Files with no reviewable changes (52)
- DashSync/shared/Models/Managers/Service Managers/DSVersionManager.m
- DashSync/shared/Models/Managers/Service Managers/DSInsightManager.m
- DashSync/shared/Models/Persistence/Migration/Policies/DSDeleteEntityMigrationPolicy.m
- DashSync/shared/Models/DAPI/NSPredicate+CBORData.m
- DashSync/shared/Models/Transactions/Provider/DSProviderUpdateRegistrarTransaction.m
- DashSync/shared/Models/Managers/Service Managers/Auth/DSAuthenticationManager.m
- DashSync/shared/Models/Wallet/DSWallet.m
- DashSync/shared/Models/Managers/Service Managers/DSShapeshiftManager.m
- DashSync/shared/Models/Payment/DSPaymentRequest.m
- DashSync/shared/Libraries/AdvancedOperations/Operations/DSOperation.m
- DashSync/shared/DashSync.m
- DashSync/shared/Models/Transactions/Base/DSTransaction.m
- DashSync/shared/Models/Wallet/DSBIP39Mnemonic.m
- DashSync/shared/Models/Managers/Chain Managers/DSIdentitiesManager.m
- DashSync/shared/Models/DAPI/DSPlatformDocumentsRequest.m
- DashSync/shared/Models/Entities/DSSimplifiedMasternodeEntryEntity+CoreDataClass.m
- DashSync/shared/Models/Masternode/DSQRInfoProcessingResult.m
- DashSync/shared/Models/Masternode/DSMasternodeListDiffService.m
- DashSync/shared/Models/Derivation Paths/DSSimpleIndexedDerivationPath.m
- DashSync/shared/Models/Masternode/DSQuorumRotationService.m
- DashSync/shared/Models/Entities/DSTxOutputEntity+CoreDataClass.m
- DashSync/shared/Models/Managers/Chain Managers/DSKeyManager.m
- DashSync/shared/Categories/NSData/NSData+Dash.m
- DashSync/shared/Models/Masternode/DSSimplifiedMasternodeEntry.m
- DashSync/shared/Models/Chain/DSMerkleBlock.m
- DashSync/shared/Models/Chain/DSFullBlock.m
- DashSync/shared/Models/Managers/Chain Managers/DSSporkManager.m
- DashSync/shared/Models/Wallet/DSSpecialTransactionsWalletHolder.m
- DashSync/shared/Models/Masternode/DSMasternodeListService.m
- DashSync/shared/Models/Derivation Paths/DSAuthenticationKeysDerivationPath.m
- DashSync/shared/Models/Governance/DSGovernanceObject.m
- DashSync/shared/Libraries/DSMerkleTree.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIPlatformNetworkService.m
- DashSync/shared/Models/Managers/Chain Managers/DSGovernanceSyncManager.m
- DashSync/shared/Models/Derivation Paths/DSIncomingFundsDerivationPath.m
- DashSync/shared/Models/Entities/DSMasternodeListEntity+CoreDataClass.m
- DashSync/shared/Models/Masternode/DSMnDiffProcessingResult.m
- DashSync/shared/Models/Masternode/DSMasternodeProcessorContext.m
- DashSync/shared/Categories/NSManagedObject+Sugar.m
- DashSync/shared/Models/Managers/Chain Managers/DSMasternodeManager+Mndiff.m
- DashSync/shared/Models/Transactions/Base/DSInstantSendTransactionLock.m
- DashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.m
- DashSync/shared/Models/Masternode/DSMasternodeListStore.m
- DashSync/shared/Models/Masternode/DSQuorumEntry.m
- DashSync/shared/Models/Identity/DSBlockchainIdentity.m
- DashSync/shared/Models/Chain/DSChainLock.m
- DashSync/shared/Models/Entities/DSChainEntity+CoreDataClass.m
- DashSync/shared/Models/Derivation Paths/DSDerivationPath.m
- DashSync/shared/Models/Chain/DSBlock.m
- DashSync/shared/Models/DAPI/Networking/DSDAPIGRPCResponseHandler.m
- DashSync/shared/Models/Transactions/Provider/DSProviderRegistrationTransaction.m
- DashSync/shared/Categories/NSManagedObjectContext+DSSugar.m
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
Example/CLAUDE.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CLAUDE.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
DashSync/shared/Models/Managers/CLAUDE.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
DashSync/shared/Models/CLAUDE.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Testnet Chain
- GitHub Check: Testnet E2E Tests
- GitHub Check: Test on iPhone simulator
- GitHub Check: Mainnet Chain
- GitHub Check: Code coverage
🔇 Additional comments (27)
DashSync/shared/Models/Payment/DSPaymentProtocol.m (2)
272-272: LGTM: Improved forward compatibility.Silently ignoring unknown protobuf fields is correct behavior per the Protocol Buffers specification, allowing this parser to handle future protocol extensions gracefully.
513-521: Consider adding logging for signature verification failures.This security-critical signature verification path currently has no logging. While error handling remains functional through
_errorMessageand the return value, adding logging with the available macros (e.g.,DSLogWarnorDSLogError) would improve debuggability for payment protocol validation issues.DashSync/shared/Models/Managers/Chain Managers/DSChainsManager.m (1)
146-335: LGTM—Whitespace cleanup aligns with broader PR goals.The removed empty lines at lines 251 and 329 are pure formatting changes with no functional impact. The devnet chain management logic remains intact.
DashSync/shared/Models/Managers/Chain Managers/DSPeerManager.m (3)
849-859: LGTM—Formatting cleanup in peer sync logic.The whitespace adjustments at lines 849, 853-854 improve code formatting without affecting the peer synchronization or mempool handling logic. The notification posting and transaction status updates remain intact.
916-920: Approved—Formatting change in sync notification.The whitespace cleanup at line 916 improves readability without affecting the transaction status notification or blockchain sync initiation logic.
933-954: LGTM—Whitespace cleanup in disconnect handling.The formatting changes at lines 949 and 952 streamline the peer disconnect and sync state logic without affecting error handling or reconnection behavior.
DashSync/shared/Models/CoinJoin/Utils/DSMasternodeGroup.m (1)
1-551: LGTM! Logging cleanup aligns with PR objectives.All changes in this file are limited to removing legacy DSLog statements and formatting cleanup (blank lines). No functional logic, state management, or peer handling has been altered. This aligns perfectly with the PR's goal of removing legacy logging infrastructure.
DashSync/shared/Models/Wallet/DSAccount.m (1)
1-1869: LGTM! Comprehensive logging cleanup with no functional changes.This file underwent extensive removal of legacy logging macros (LOG_BALANCE_UPDATE, TEST_LOG_ALL_TRANSACTIONS, etc.) without any alterations to:
- Balance calculation logic
- Transaction handling and verification
- Signing operations
- Persistence or Core Data operations
All control flow and state management remain unchanged. This cleanup aligns with the PR's objective to migrate from legacy DSLog infrastructure to the new DSLogger system.
DashSync/shared/Models/Managers/Chain Managers/DSChainManager.m (3)
28-28: LGTM! Proper logging infrastructure import.Added import of DSLogger.h to support the new Android-style logging system introduced in this PR.
129-132: LGTM! Clean migration to structured logging.Properly migrated from legacy DSLog to DSLogInfo with class names ("DSChainManager"), providing structured, thread-aware logging for sync lifecycle events. The format will now match the Android-style "HH:MM:SS [thread-name] ClassName - message" pattern described in the PR objectives.
Also applies to: 135-140
276-290: LGTM! Improved sync completion logging.Added DSLogInfo for chain sync completion with the current height, providing better observability of sync milestones. The logging cleanup preserves all functional behavior (mempool fetch, spork retrieval, governance sync, masternode sync).
DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (2)
60-76: LGTM! Well-implemented per-chain singleton pattern.The new
sharedInstanceForChain:method properly implements a thread-safe singleton factory pattern:
- Uses
dispatch_onceto initialize the shared dictionary- Synchronizes access to the dictionary with
@synchronized- Indexes managers by
chain.uniqueIDfor per-chain isolation- Returns existing instances or creates new ones as needed
This aligns with the broader architectural pattern of managing CoinJoin operations per chain.
78-950: LGTM! Comprehensive logging cleanup with no functional changes.All modifications beyond the new singleton method are limited to:
- Removal of legacy DSLog/DSLogPrivate statements
- Minor formatting cleanup (blank lines)
No changes to:
- CoinJoin mixing logic and state management
- Transaction handling and signing
- UTXO selection and coin control
- Masternode communication
- Balance calculations
The control flow and business logic remain completely unchanged. This cleanup aligns with the PR's objective to migrate to the DSLogger infrastructure.
DashSync/shared/Libraries/DSLogger.h (2)
28-51: LGTM! Well-designed Android-style logging macros.The logging macro design is clean and follows CocoaLumberjack conventions properly. The
##__VA_ARGS__usage correctly handles zero variadic arguments, and the conditional compilation ofDSLogVerbosefor DEBUG-only builds is appropriate.One minor observation: the format string shows
"HH:MM:SS [thread] ClassName - message"in the comment (Line 34), but the formatter in DSLogger.m usesHH:mm:ss. The comment should match the actual format for consistency.
71-76: LGTM!The new
log:className:method provides a clean API for programmatic logging with class context.DashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.m (3)
164-164: LGTM! Comprehensive transaction lifecycle logging.The logging additions provide excellent visibility into transaction broadcasting, relay tracking, and lock verification. The consistent use of
uint256_reverse_hex()for transaction hashes and the inclusion of peer counts will aid debugging significantly.
1431-1445: LGTM! Good security-relevant logging for InstantSend locks.Logging both reception and verification status of InstantSend locks is valuable for debugging and auditing. The format is consistent with other transaction logs.
1607-1611: LGTM! ChainLock verification logging.Consistent with the InstantSend logging pattern, providing good visibility into ChainLock verification status.
DashSync/shared/Libraries/DSLogger.m (2)
88-102: LGTM! Logger initialization is well-structured.The formatter instance can be safely shared between console and file loggers since
DSAndroidStyleLogFormattermaintains no mutable instance state. The log file configuration (5 MB max, 24-hour rolling, 10 files retained) aligns with the PR objectives.
52-64: Implementation is thread-safe and idiomatic for iOS 13.0+.Static
NSDateFormatterwithdispatch_onceis the correct pattern. Since this codebase targets iOS 13.0 minimum,NSDateFormatteris guaranteed thread-safe for concurrentstringFromDatecalls, making the current implementation safe without additional synchronization.DashSync/shared/Models/Network/DSPeer.m (4)
219-219: LGTM! Peer connection logging.Good logging of peer connection initiation with host and port information.
308-312: LGTM! Differentiated disconnect logging.Using
DSLogWarnfor error disconnects andDSLogInfofor clean disconnects is a good practice that will help distinguish problematic disconnections in log analysis.
354-355: LGTM! Connected peer details logging.Logging version, services, and useragent on successful connection provides valuable diagnostic information without exposing sensitive data.
773-774: LGTM! Version message logging.Logging received version details at DEBUG level is appropriate—useful for debugging but not overly verbose in production.
DashSync/shared/Models/Chain/DSChain.m (3)
27-27: LGTM: Logger import added for new logging functionality.The import of
DSLogger.his consistent with the PR's objective to implement Android-style logging throughout the codebase.
1780-1781: Verify nil handling for peer.host in log statement.The log statement directly uses
peer.hostwithout a nil check. Line 1776 shows thatpeer.hostcan be nil (handled with a ternary operator:peer.host ? peer.host : @"TEST"). While logging(null)won't crash in Objective-C, it may result in less useful log output.Consider using consistent nil handling:
💡 Suggested improvement
- DSLogDebug(@"DSChain", @"received block %@ at height %u from peer %@", - uint256_reverse_hex(block.blockHash), block.height, peer.host); + DSLogDebug(@"DSChain", @"received block %@ at height %u from peer %@", + uint256_reverse_hex(block.blockHash), block.height, peer.host ?: @"unknown");
2045-2045: LGTM: Comments improve clarity of chainlock logic.The added comments clearly document scenarios where chain reorganization is prevented due to chainlocks, improving code maintainability and making the complex blockchain logic more understandable.
Also applies to: 2276-2276, 2327-2327
| ``` | ||
| DashSync/ | ||
| ├── DashSync/shared/ # Main framework source (cross-platform) | ||
| │ ├── Models/ # Core domain models (24 subdirectories) | ||
| │ ├── Libraries/ # Utility libraries | ||
| │ └── DashSync.xcdatamodeld/ # Core Data model (83 entities) | ||
| ├── DashSync/iOS/ # iOS-specific code | ||
| ├── DashSync/macOS/ # macOS-specific code | ||
| ├── Example/ # Reference app and tests | ||
| ├── Scripts/ # Build utilities | ||
| └── ChainResources/ # Blockchain data files | ||
| ``` |
There was a problem hiding this comment.
Specify language identifier for project structure code block.
The fenced code block for the directory structure should declare a language (e.g., text or plaintext) for markdown linting consistency.
📝 Proposed fix
-```
+```text
DashSync/
├── DashSync/shared/ # Main framework source (cross-platform)
│ ├── Models/ # Core domain models (24 subdirectories)📝 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.
| ``` | |
| DashSync/ | |
| ├── DashSync/shared/ # Main framework source (cross-platform) | |
| │ ├── Models/ # Core domain models (24 subdirectories) | |
| │ ├── Libraries/ # Utility libraries | |
| │ └── DashSync.xcdatamodeld/ # Core Data model (83 entities) | |
| ├── DashSync/iOS/ # iOS-specific code | |
| ├── DashSync/macOS/ # macOS-specific code | |
| ├── Example/ # Reference app and tests | |
| ├── Scripts/ # Build utilities | |
| └── ChainResources/ # Blockchain data files | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @CLAUDE.md around lines 38 - 49, Add a language identifier to the fenced code
block that contains the DashSync directory tree (the block starting with
"DashSync/") so markdown linters recognize it as plain text; update the opening
triple-backticks to include a language token such as "text" or "plaintext"
(e.g., change ``` to ```text) while keeping the directory tree content
unchanged.
| ``` | ||
| DSChain (root object) | ||
| ├── DSWallet[] | ||
| │ └── DSAccount[] | ||
| │ └── DSDerivationPath[] | ||
| ├── DSBlock[] / DSMerkleBlock[] | ||
| ├── DSMasternodeList[] | ||
| │ └── DSSimplifiedMasternodeEntry[] | ||
| ├── DSQuorum[] | ||
| └── DSSpork[] | ||
| ``` |
There was a problem hiding this comment.
Specify language identifiers for hierarchy and structure code blocks.
Three fenced code blocks (model hierarchy, manager hierarchy, and transaction inheritance) lack language specifications and should declare text or plaintext for markdown consistency.
📝 Proposed fixes
Line 35 (Model Hierarchy):
-```
+```text
DSChain (root object)
├── DSWallet[]Line 49 (Manager Hierarchy):
-```
+```text
DSChainsManager (singleton)
└── DSChainManager (per chain)Line 65 (Transaction Inheritance):
-```
+```text
DSTransaction (base)
├── DSCoinbaseTransactionAlso applies to: 49-59, 65-76
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @DashSync/shared/Models/CLAUDE.md around lines 35 - 45, The three Markdown
fenced code blocks showing the model hierarchy, manager hierarchy, and
transaction inheritance should include a language identifier (e.g., text or
plaintext) for consistency; update the block that begins with "DSChain (root
object)" (references to DSChain, DSWallet, DSAccount, DSDerivationPath), the
block that begins with "DSChainsManager (singleton)" (references to
DSChainsManager, DSChainManager), and the block that begins with "DSTransaction
(base)" (references to DSTransaction, DSCoinbaseTransaction) by changing their
opening fences from ``` to ```text (or ```plaintext).
DashSync/shared/Models/Persistence/Migration/DSCoreDataMigrator.m
Outdated
Show resolved
Hide resolved
| ``` | ||
| Example/ | ||
| ├── DashSync/ # Example app source code | ||
| ├── Tests/ # Unit and integration tests | ||
| ├── NetworkInfo/ # Network configuration | ||
| ├── DashSync.xcworkspace # Xcode workspace (use this!) | ||
| ├── DashSync.xcodeproj/ # Xcode project | ||
| ├── Podfile # CocoaPods configuration | ||
| └── Podfile.lock # Locked dependency versions | ||
| ``` |
There was a problem hiding this comment.
Specify language identifier for directory structure code block.
The fenced code block for the directory structure should declare a language (e.g., text or plaintext) for consistency with markdown linting standards.
📝 Proposed fix
-```
+```text
Example/
├── DashSync/ # Example app source code
├── Tests/ # Unit and integration tests
├── NetworkInfo/ # Network configuration
├── DashSync.xcworkspace # Xcode workspace (use this!)
├── DashSync.xcodeproj/ # Xcode project
├── Podfile # CocoaPods configuration
└── Podfile.lock # Locked dependency versions
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @Example/CLAUDE.md around lines 22 - 31, The code block showing the directory
tree in Example/CLAUDE.md lacks a language identifier, which trips markdown
linters; update the fenced block opening from totext (or ```plaintext)
so the directory structure is declared as plain text (i.e., change the code
fence around the Example/ directory listing to include "text").
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
| #if DEBUG | ||
| DSLogPrivate(@"[%@] CoinJoin tx: Mixing Fee: %@", self.chain.name, uint256_reverse_hex(lastTransaction.txHash)); | ||
| #else | ||
| DSLog(@"[%@] CoinJoin tx: Mixing Fee: %@", self.chain.name, @"<REDACTED>"); | ||
| #endif |
There was a problem hiding this comment.
@bfoss765 This one example of many where the logging is removed, but I think should remain.
Some of the logging doesn't line up with Android (DashJ) because the architecture and processes are different, but we would still want to know about these things in the logs.
| #if DEBUG | ||
| DSLogPrivate(@"received input lockTime %d for transaction %@", tx.lockTime, [NSData dataWithUInt256:tx.txHash].reverse.hexString); | ||
| #else | ||
| DSLog(@"received input lockTime %d for transaction %@", tx.lockTime, @"<REDACTED>"); | ||
| #endif |
There was a problem hiding this comment.
We want to keep this logging for sure as it relates to receiving a transaction.
| #if DEBUG | ||
| DSLogPrivate(@"[%@] [DSAccount] registering transaction %@", self.wallet.chain.name, transaction); | ||
| #else | ||
| DSLog(@"[%@] [DSAccount] registering transaction %@", self.wallet.chain.name, @"<REDACTED>"); | ||
| #endif |
There was a problem hiding this comment.
Logging here is also necessary.
HashEngineering
left a comment
HashEngineering
left a comment
There was a problem hiding this comment.
I left some comments. Many log statements were removed.
Instead of removing them, we should keep them and perhaps rewrite them to follow the new guidelines.
Some comments were only for debugging in a debug build, but others related to receiving transactions, coinjoin, quorums and other related functionality.
- Fix UTC timezone for log timestamps - Add iOS reference to log filenames (ios-YYYY-MM-DD--HH-mm-ss.log) - Fix DSCoinJoinWrapper boolean inversion in getCoinJoinKeys - Fix DSCoinJoinWrapper memory leak (double call to script_pubkey_for_address) - Fix DSCoreDataMigrator error handling for backup exclusion - Add transaction creation logging with outputs, inputs, amount, fee - Add transaction list logging after sync completes - Add CoinJoin mixing fee transaction detection logging - Add lockTime to received transaction logs - Add UTXO balance update logging - Add transaction registration logging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (1)
163-166: Add nil check for account to prevent potential crash.The
firstAccountThatCanContainTransaction:method can return nil if no account contains the transaction. Passing nil to the second method will cause a crash whenamountsSentByTransaction:is called on nil at line 169.🛡️ Proposed fix with nil check
+ (CoinJoinTransactionType)coinJoinTxTypeForTransaction:(DSTransaction *)transaction { DSAccount *account = [transaction.chain firstAccountThatCanContainTransaction:transaction]; + if (!account) { + return CoinJoinTransactionType_Standard; // or appropriate default + } return [DSCoinJoinWrapper coinJoinTxTypeForTransaction:transaction account:account]; }
🤖 Fix all issues with AI agents
In @DashSync/shared/Libraries/DSLogger.m:
- Around line 34-42: The current thread ID extraction in DSLogger.m relies on
parsing NSThread.description (the code using thread.description and
rangeOfString:@"number = "), which is undocumented and fragile; replace that
parsing logic with a stable identifier by calling
pthread_mach_thread_np(pthread_self()) (or use it as a fallback when parsing
fails) and format the result into the same "Thread-<id>" string expected by the
logger; update the method that generates the thread label (where
thread.description is used) to prefer pthread_mach_thread_np(pthread_self()) and
preserve the existing @"Thread" fallback only if obtaining the pthread id
somehow fails.
🧹 Nitpick comments (3)
DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (1)
58-77: Consider lifecycle management for the per-chain singleton dictionary.The static
_managerChainDictionaryaccumulates manager instances without ever removing them. If chains are created and destroyed dynamically (e.g., during testing or network switching), this will cause memory accumulation.♻️ Consider adding a cleanup method
If dynamic chain lifecycle is a concern, consider adding a class method to remove managers for deallocated chains:
+ (void)removeManagerForChain:(DSChain *)chain { NSParameterAssert(chain); @synchronized(_managerChainDictionary) { [_managerChainDictionary removeObjectForKey:chain.uniqueID]; } }Alternatively, use
NSMapTablewith weak values to automatically release managers when chains are deallocated.DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (1)
168-183: Consider defensive malloc error handling.While malloc failures are rare, adding a nil check for
inputValueswould make the code more robust and prevent potential crashes in low-memory scenarios.🛡️ Proposed defensive check
+ (CoinJoinTransactionType)coinJoinTxTypeForTransaction:(DSTransaction *)transaction account:(DSAccount *)account { NSArray *amountsSent = [account amountsSentByTransaction:transaction]; Transaction *tx = [transaction ffi_malloc:transaction.chain.chainType]; uint64_t *inputValues = malloc(amountsSent.count * sizeof(uint64_t)); + if (!inputValues) { + [DSTransaction ffi_free:tx]; + return CoinJoinTransactionType_Standard; // or appropriate default + } for (uintptr_t i = 0; i < amountsSent.count; i++) { inputValues[i] = [amountsSent[i] unsignedLongLongValue]; } CoinJoinTransactionType type = get_coinjoin_tx_type(tx, inputValues, amountsSent.count); [DSTransaction ffi_free:tx]; free(inputValues); return type; }DashSync/shared/Models/Managers/Chain Managers/DSChainManager.m (1)
283-291: Consider performance impact with large wallets.The post-sync transaction logging iterates through all transactions in all wallets. For wallets with thousands of transactions, this could introduce noticeable delay and log volume.
The use of
DSLogDebugfor individual transactions is appropriate (verbose level), but consider:
- Adding a transaction count limit or sampling for very large wallets
- Ensuring this doesn't block critical sync completion paths
- The logging occurs on the chain's networking queue, which may be acceptable for post-sync operations
Optional: Add transaction limit for logging
// Log transaction list after sync completion for (DSWallet *wallet in chain.wallets) { NSArray *transactions = wallet.allTransactions; DSLogInfo(@"DSChainManager", @"Wallet has %lu transactions after sync", (unsigned long)transactions.count); - for (DSTransaction *tx in transactions) { + NSUInteger maxToLog = MIN(transactions.count, 100); // Limit to first 100 + for (NSUInteger i = 0; i < maxToLog; i++) { + DSTransaction *tx = transactions[i]; NSString *status = tx.blockHeight == TX_UNCONFIRMED ? @"unconfirmed" : [NSString stringWithFormat:@"confirmed at %u", tx.blockHeight]; DSLogDebug(@"DSChainManager", @" TX %@ - %@", uint256_reverse_hex(tx.txHash), status); } + if (transactions.count > maxToLog) { + DSLogDebug(@"DSChainManager", @" ... and %lu more transactions", (unsigned long)(transactions.count - maxToLog)); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
DashSync/shared/Libraries/DSLogger.mDashSync/shared/Libraries/Logs/CompressingLogFileManager.mDashSync/shared/Models/CoinJoin/DSCoinJoinManager.mDashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.mDashSync/shared/Models/Managers/Chain Managers/DSChainManager.mDashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.mDashSync/shared/Models/Persistence/Migration/DSCoreDataMigrator.mDashSync/shared/Models/Wallet/DSAccount.m
🚧 Files skipped from review as they are similar to previous changes (2)
- DashSync/shared/Models/Wallet/DSAccount.m
- DashSync/shared/Models/Persistence/Migration/DSCoreDataMigrator.m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Testnet Chain
- GitHub Check: Mainnet Chain
- GitHub Check: Testnet E2E Tests
- GitHub Check: Objective-C Linter
- GitHub Check: Test on iPhone simulator
🔇 Additional comments (20)
DashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.m (8)
28-28: LGTM: DSLogger import and transaction publishing logs.The addition of DSLogger.h and new logging statements effectively trace transaction publishing lifecycle with peer counts and transaction hashes, providing valuable diagnostics without altering functionality.
Also applies to: 164-164, 178-179, 218-218
394-394: LGTM: Transaction timeout warning.Appropriate use of
DSLogWarnfor broadcast timeout with transaction hash context.
1035-1035: LGTM: Peer transaction request logging.Clear logging of inbound transaction requests with peer and transaction context aids network propagation tracking.
1122-1122: LGTM: Transaction relay tracking logs.Comprehensive relay tracking with peer announcements, relay counts, and network confirmation thresholds provides excellent visibility into transaction propagation and acceptance.
Also applies to: 1139-1140, 1143-1146
1171-1177: LGTM: Transaction reception logging with lockTime.Enhanced logging for transaction reception includes lockTime, block context, and peer information as specified in PR objectives, providing complete transaction lifecycle visibility.
1431-1432: LGTM: InstantSend lock logging.Comprehensive InstantSend logging captures lock reception and verification status with appropriate log levels (Info for events, Debug for duplicates), directly supporting PR objectives for InstantSend observability.
Also applies to: 1438-1439, 1444-1445
1607-1611: LGTM: ChainLock logging.ChainLock reception and verification logging includes block hash, height, and peer context, meeting PR objectives for comprehensive ChainLock observability.
1363-1363: LGTM: Transaction rejection logging.Appropriate warning-level logging for peer transaction rejections with complete context including rejection code.
DashSync/shared/Models/CoinJoin/DSCoinJoinManager.m (1)
269-277: LGTM!The DSLogInfo statements appropriately log CoinJoin transaction detection events with clear messages and proper transaction hash formatting.
DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (1)
641-667: LGTM!The
getCoinJoinKeysfunction correctly selects addresses based on theusedparameter and properly constructs the FFICoinJoinKeysstructure with appropriate memory management.DashSync/shared/Libraries/DSLogger.m (4)
46-48: LGTM!The formatter interface is properly declared and conforms to the
DDLogFormatterprotocol.
52-65: LGTM!The formatter implementation correctly:
- Uses
dispatch_oncefor thread-safe singleton initialization of the date formatter (good for performance)- Explicitly sets UTC timezone matching the Android logging format
- Reuses the formatter instance across calls
86-109: LGTM!The logger initialization correctly:
- Creates and applies the Android-style formatter to both console and file loggers
- Configures file rotation parameters matching the PR objectives (5 MB max, 24-hour rolling, 10 retained files)
- Uses
CompressingLogFileManagerfor automatic gzip compression
132-138: LGTM!The logging methods correctly:
- Replace legacy
DSLogwithDDLogInfofor the simple message variant- Add a new contextual logging method that uses the
DSLogInfomacro with class nameDashSync/shared/Models/Managers/Chain Managers/DSChainManager.m (4)
28-28: LGTM!The import is necessary for the new logging macros used throughout the file.
129-129: LGTM!The logging statement correctly uses
DSLogInfowith appropriate context (chain name) for this sync lifecycle event.
135-135: LGTM!The logging statement correctly uses
DSLogInfofor this sync lifecycle event, matching the pattern fromstartSync.
281-281: LGTM!The sync completion log correctly includes the final chain height, providing valuable diagnostic information matching the PR objectives.
DashSync/shared/Libraries/Logs/CompressingLogFileManager.m (2)
77-85: LGTM! Clean UTC timestamp implementation.The UTC timezone configuration and filename format correctly implement the iOS-specific log naming convention described in the PR objectives. The date format matches the expected pattern and will ensure consistent timestamps across devices regardless of local timezone.
87-94: Excellent backward compatibility implementation.Supporting both "log-" and "ios-" prefixes ensures a smooth transition from the legacy format to the new Android-style naming convention. This allows existing compressed log files to continue being recognized while new files use the updated format.
| NSString *description = thread.description; | ||
| NSRange numRange = [description rangeOfString:@"number = "]; | ||
| if (numRange.location != NSNotFound) { | ||
| NSUInteger start = numRange.location + numRange.length; | ||
| NSRange endRange = [description rangeOfString:@"," options:0 range:NSMakeRange(start, description.length - start)]; | ||
| if (endRange.location != NSNotFound) { | ||
| return [NSString stringWithFormat:@"Thread-%@", [description substringWithRange:NSMakeRange(start, endRange.location - start)]]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "DSLogger.m" -o -name "DSLogger.h"Repository: dashpay/dashsync-iOS
Length of output: 141
🏁 Script executed:
cat -n ./DashSync/shared/Libraries/DSLogger.m | head -50Repository: dashpay/dashsync-iOS
Length of output: 2239
🏁 Script executed:
# Check if there are any other references to thread names or IDs in the codebase
rg -i "thread" ./DashSync/shared/Libraries/DSLogger.m -A 2 -B 2
# Also check for any pthread or thread-related imports
rg "pthread|Foundation|Darwin" ./DashSync/shared/Libraries/DSLogger.mRepository: dashpay/dashsync-iOS
Length of output: 1190
🏁 Script executed:
# Check the imports at the top of the file
head -25 ./DashSync/shared/Libraries/DSLogger.m | grep -E "^#import|^#include"
# Check if pthread is used elsewhere in this file
rg "pthread" ./DashSync/shared/Libraries/DSLogger.mRepository: dashpay/dashsync-iOS
Length of output: 122
🏁 Script executed:
cat -n ./DashSync/shared/Libraries/DSLogger.hRepository: dashpay/dashsync-iOS
Length of output: 3385
🏁 Script executed:
# Check if there are any comments or references to NSThread format elsewhere
rg "NSThread\|description\|number = " ./DashSync --type objc --type m -B 2 -A 2 | head -40
# Check if pthread is available in Foundation/Darwin headers
rg "pthread" ./DashSync/shared --type h --type m | head -20Repository: dashpay/dashsync-iOS
Length of output: 115
🏁 Script executed:
# Search for pthread usage in codebase
rg "pthread" ./DashSync --type objc -B 2 -A 2 | head -40
# Also check if there are any existing alternatives for getting thread IDs
rg "threadNumber\|threadId\|mach_thread" ./DashSync --type objcRepository: dashpay/dashsync-iOS
Length of output: 46
🏁 Script executed:
# Check if NSThread.description format has been an issue or if there are any version-specific comments
rg "description\|format\|version\|iOS" ./DashSync/shared/Libraries/DSLogger.m -B 1 -A 1
# Check the full DSLogger.m file to see if there are any other concerns
wc -l ./DashSync/shared/Libraries/DSLogger.mRepository: dashpay/dashsync-iOS
Length of output: 106
🏁 Script executed:
cat -n ./DashSync/shared/Libraries/DSLogger.mRepository: dashpay/dashsync-iOS
Length of output: 5854
Use a stable thread identifier instead of parsing undocumented description format.
The thread number extraction relies on the undocumented NSThread.description format, which Apple could change. While the code gracefully falls back to @"Thread", parsing failures lose useful debugging information. The defensive fallback check prevents crashes but still leaves the code fragile.
Consider using pthread_mach_thread_np(pthread_self()) as a more stable fallback that provides consistent thread identification across iOS versions.
🤖 Prompt for AI Agents
In @DashSync/shared/Libraries/DSLogger.m around lines 34 - 42, The current
thread ID extraction in DSLogger.m relies on parsing NSThread.description (the
code using thread.description and rangeOfString:@"number = "), which is
undocumented and fragile; replace that parsing logic with a stable identifier by
calling pthread_mach_thread_np(pthread_self()) (or use it as a fallback when
parsing fails) and format the result into the same "Thread-<id>" string expected
by the logger; update the method that generates the thread label (where
thread.description is used) to prefer pthread_mach_thread_np(pthread_self()) and
preserve the existing @"Thread" fallback only if obtaining the pthread id
somehow fails.
- Enable auto-review for PRs and incremental pushes - Enable auto-resolve for comments when fixes are pushed - Configure concise review profile - Enable chat auto-reply 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.coderabbit.yaml:
- Around line 15-21: Change the invalid reviews.profile value to one of the
allowed enums (use "chill" or "assertive", e.g., set reviews.profile: "chill")
and remove the reviews.auto_resolve block (delete the auto_resolve key under
reviews) since the schema disallows unknown keys; if you actually need
auto_resolve, move it to the schema-allowed location or top-level key that the
schema supports instead of reviews.auto_resolve.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.coderabbit.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Testnet Chain
- GitHub Check: Mainnet Chain
- GitHub Check: Testnet E2E Tests
- GitHub Check: Test on iPhone simulator
- GitHub Check: Objective-C Linter
🔇 Additional comments (1)
.coderabbit.yaml (1)
4-14: Config looks schema-aligned for language, auto-review gating, base branches, and chat auto-reply.No schema issues spotted in these sections per the provided schema.
Also applies to: 22-27
- Change profile from 'concise' to 'chill' (valid enum) - Remove invalid auto_resolve key from reviews block 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit adds comprehensive logging improvements to match Android/DashJ log output for easier debugging and comparison across platforms. Changes: - Add DSChainSyncSpeedCalculator for periodic sync stats logging - Blocks/sec, tx/sec, pre-filtered tx/sec, headers/sec, mnlistdiff/sec - Average/last KB/sec throughput - Chain height vs target, stall detection - Add wallet save timing to Core Data operations - Add UTXO state change logging in DSAccount - Log when outputs are marked as spent - Log prevtx state transitions - Add transaction amount to receive logs (matching Android format) - Add Bloom filter event logging - Filter send to peer - Filter rebuild timing - Add key derivation timing logging in DSFundsDerivationPath - Add peer group statistics to connection logs - Connected/pending/max counts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix incorrect [NSDate timeIntervalSince1970] calls to use [[NSDate date] timeIntervalSince1970] (instance method, not class) - Add app version and DashSync version logging at startup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@DashSync/shared/Categories/NSManagedObjectContext`+DSSugar.m:
- Around line 68-94: ds_save currently logs "Save completed" and timing/counts
unconditionally even when the save (the -[NSManagedObjectContext save:&error]
call in ds_save) fails; modify ds_save so that the DSLogInfo call (and the
computed saveTime/insertedCount/updatedCount/deletedCount logging) occurs only
when save:&error returns YES (i.e., after a successful save), while still
ensuring the background task is ended and the NSError is returned on failure;
locate the save call and DSLogInfo in ds_save and wrap the logging in the
success branch so failures do not produce a "Save completed" message.
In `@DashSync/shared/Models/Derivation` Paths/DSFundsDerivationPath.m:
- Around line 184-231: The else branch is unreachable because keysNeeded =
gapLimit - a.count is always > 0 when we get past the earlier return, so
duplicate address-generation logic should be consolidated: remove the entire
else branch (lines with the second while block) and keep a single
address-generation loop that uses the existing logic (call to
publicKeyDataAtIndex:internal:, DSKeyManager
ecdsaKeyAddressFromPublicKeyData:forChainType:, checks for nil error, adding to
mAllAddresses and either internalAddresses or externalAddresses, appending to a
and addAddresses, and incrementing n); retain the DSLogInfo timing/logging code
only when keysNeeded > 0 (i.e., compute keysNeeded, conditionally log, then run
the single shared while (a.count < gapLimit) loop).
In `@DashSync/shared/Models/Managers/Chain` Managers/DSChainManager.m:
- Around line 287-295: The per-transaction logging in DSChainManager iterates
wallet.allTransactions and logs every DSTransaction (using uint256_reverse_hex
and DSLogDebug), which can hurt performance and leak sensitive data; modify the
block so that DSLogInfo still reports the total count but per-transaction
details are either gated behind a debug/verbose flag (check a global
verbose/debug setting) or limited to a sample (e.g., only log the last 10
transactions), and change DSLogDebug to DSLogVerbose for those detailed lines;
ensure you still preserve the TX_UNCONFIRMED check and message formatting when
emitting the gated/limited logs.
In `@DashSync/shared/Models/Managers/Chain` Managers/DSChainSyncSpeedCalculator.m:
- Around line 259-291: reset currently uses dispatch_async on self.statsQueue
which defers clearing state and can leave stale values; change the
implementation of - (void)reset to use dispatch_sync(self.statsQueue, ^{ ... })
instead of dispatch_async so the reset completes before the method returns;
leave resetIntervalCounters as-is (still using dispatch_async) and ensure you
only modify the dispatch call in the -reset method that sets blocksThisInterval,
transactionsThisInterval, preFilteredTxThisInterval, headersThisInterval,
mnListDiffsThisInterval, bytesThisInterval, totalBlocks, totalTransactions,
totalBytes, startTime, avgKBPerSec, lastKBPerSec, chainHeight, commonHeight,
targetHeight, lastProgressTime, lastChainHeight and historyBuffer removal.
- Around line 75-87: In startCalculating, avoid adding the same NSTimer twice:
replace the scheduledTimerWithTimeInterval: call with the non-scheduling
initializer timerWithTimeInterval:target:selector:userInfo:repeats: and then add
that timer to the run loop with addTimer:forMode: (so the timer is created
without being auto-scheduled and then registered for NSRunLoopCommonModes); keep
the existing invalidate of self.logTimer and assignments to
self.startTime/self.lastProgressTime and ensure the created timer is stored to
self.logTimer and uses the selector timerFired.
♻️ Duplicate comments (1)
DashSync/shared/Libraries/DSLogger.m (1)
23-44: Thread name extraction relies on undocumentedNSThread.descriptionformat.This concern was raised in a previous review. The parsing of
thread.descriptionto extract the thread number is fragile since Apple doesn't document this format. While the fallback to@"Thread"prevents crashes, it loses debugging value when the format changes.Consider using
pthread_mach_thread_np(pthread_self())for a stable thread identifier:♻️ Suggested improvement
+#import <pthread.h> + NSString *DSCurrentThreadName(void) { NSThread *thread = [NSThread currentThread]; NSString *name = thread.name; if (name.length > 0) { return name; } if ([thread isMainThread]) { return @"main"; } - // Use thread number as fallback - NSString *description = thread.description; - NSRange numRange = [description rangeOfString:@"number = "]; - if (numRange.location != NSNotFound) { - NSUInteger start = numRange.location + numRange.length; - NSRange endRange = [description rangeOfString:@"," options:0 range:NSMakeRange(start, description.length - start)]; - if (endRange.location != NSNotFound) { - return [NSString stringWithFormat:@"Thread-%@", [description substringWithRange:NSMakeRange(start, endRange.location - start)]]; - } - } - return @"Thread"; + // Use pthread ID for stable thread identification + mach_port_t threadId = pthread_mach_thread_np(pthread_self()); + return [NSString stringWithFormat:@"Thread-%u", threadId]; }
🧹 Nitpick comments (7)
DashSync/shared/Libraries/DSLogger.h (1)
48-57: Legacy macros appropriately marked for deprecation.The comment clearly indicates these are deprecated and will be removed after migration. Consider adding a compiler deprecation warning to help identify remaining usages:
💡 Suggestion for deprecation warning
`#pragma` mark - Legacy logging macros (deprecated - for backward compatibility during migration) // These will be removed after full migration to Android-style logging -#define DSLog(frmt, ...) DDLogInfo(frmt, ##__VA_ARGS__) +#define DSLog(frmt, ...) \ + _Pragma("clang diagnostic push") \ + _Pragma("clang diagnostic warning \"-Wdeprecated-declarations\"") \ + DDLogInfo(frmt, ##__VA_ARGS__) \ + _Pragma("clang diagnostic pop")DashSync/shared/Models/Network/DSPeer.m (1)
774-775: Version message logged at DEBUG level.Consider using INFO level for version receipt since this is a key handshake event. DEBUG may be too low for production troubleshooting of peer connectivity issues.
DashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.m (1)
1172-1190: Transaction amount logging provides useful diagnostics.The amount calculation correctly computes net wallet impact (received - sent). Using 100000000.0 for satoshi-to-DASH conversion is correct.
Minor improvement: Consider extracting the satoshi conversion constant:
💡 Suggested improvement
// At file scope or in a constants header static const double DUFFS_PER_DASH = 100000000.0; // Usage double txAmountDash = (double)txAmount / DUFFS_PER_DASH;DashSync/shared/Libraries/DSLogger.m (1)
86-112: Shared formatter instance between console and file loggers may cause issues.The same
DSAndroidStyleLogFormatterinstance is assigned to both the OS logger and the file logger. WhileDDLogFormatterinstances are typically thread-safe for read-only formatters like this one, CocoaLumberjack documentation recommends using separate formatter instances per logger to avoid potential issues.♻️ Suggested fix
- (instancetype)init { self = [super init]; if (self) { - DSAndroidStyleLogFormatter *formatter = [[DSAndroidStyleLogFormatter alloc] init]; // Console logger with formatter DDOSLogger *osLogger = [DDOSLogger sharedInstance]; - [osLogger setLogFormatter:formatter]; + [osLogger setLogFormatter:[[DSAndroidStyleLogFormatter alloc] init]]; [DDLog addLogger:osLogger]; // File logger with formatter unsigned long long maxFileSize = 1024 * 1024 * 5; // 5 MB max CompressingLogFileManager *logFileManager = [[CompressingLogFileManager alloc] initWithFileSize:maxFileSize]; DDFileLogger *fileLogger = [[DDFileLogger alloc] initWithLogFileManager:logFileManager]; fileLogger.rollingFrequency = 60 * 60 * 24; // 24 hour rolling fileLogger.maximumFileSize = maxFileSize; fileLogger.logFileManager.maximumNumberOfLogFiles = 10; - [fileLogger setLogFormatter:formatter]; + [fileLogger setLogFormatter:[[DSAndroidStyleLogFormatter alloc] init]];DashSync/shared/Models/Managers/Chain Managers/DSChainSyncSpeedCalculator.m (2)
20-51: Redundant thread-safety: atomic properties with serial queue.The properties are marked
atomicbut all access is dispatched tostatsQueue. Using both is redundant and can create confusion. Since the serial queue provides the synchronization, the properties should benonatomicfor clarity and minor performance benefit.♻️ Suggested fix
-@property (atomic, assign) NSUInteger blocksThisInterval; -@property (atomic, assign) NSUInteger transactionsThisInterval; -@property (atomic, assign) NSUInteger preFilteredTxThisInterval; -@property (atomic, assign) NSUInteger headersThisInterval; -@property (atomic, assign) NSUInteger mnListDiffsThisInterval; -@property (atomic, assign) NSUInteger bytesThisInterval; +@property (nonatomic, assign) NSUInteger blocksThisInterval; +@property (nonatomic, assign) NSUInteger transactionsThisInterval; +@property (nonatomic, assign) NSUInteger preFilteredTxThisInterval; +@property (nonatomic, assign) NSUInteger headersThisInterval; +@property (nonatomic, assign) NSUInteger mnListDiffsThisInterval; +@property (nonatomic, assign) NSUInteger bytesThisInterval; // Cumulative counters for averaging -@property (atomic, assign) NSUInteger totalBlocks; -@property (atomic, assign) NSUInteger totalTransactions; -@property (atomic, assign) NSUInteger totalBytes; -@property (atomic, assign) NSTimeInterval startTime; +@property (nonatomic, assign) NSUInteger totalBlocks; +@property (nonatomic, assign) NSUInteger totalTransactions; +@property (nonatomic, assign) NSUInteger totalBytes; +@property (nonatomic, assign) NSTimeInterval startTime; // Rolling averages -@property (atomic, assign) double avgKBPerSec; -@property (atomic, assign) double lastKBPerSec; +@property (nonatomic, assign) double avgKBPerSec; +@property (nonatomic, assign) double lastKBPerSec; // Chain heights for stall detection -@property (atomic, assign) uint32_t chainHeight; -@property (atomic, assign) uint32_t commonHeight; -@property (atomic, assign) uint32_t targetHeight; -@property (atomic, assign) NSTimeInterval lastProgressTime; -@property (atomic, assign) uint32_t lastChainHeight; +@property (nonatomic, assign) uint32_t chainHeight; +@property (nonatomic, assign) uint32_t commonHeight; +@property (nonatomic, assign) uint32_t targetHeight; +@property (nonatomic, assign) NSTimeInterval lastProgressTime; +@property (nonatomic, assign) uint32_t lastChainHeight; // History for statistics logging (ring buffer of last 10 intervals) `@property` (nonatomic, strong) NSMutableArray<NSDictionary *> *historyBuffer; -@property (atomic, assign) NSUInteger historyIndex; +@property (nonatomic, assign) NSUInteger historyIndex;
172-239: Potential deadlock:dispatch_synctostatsQueuewhile holding implicit locks.
logCurrentStatscallsdispatch_sync(self.statsQueue, ...)to read counters, then later callsdispatch_async(self.statsQueue, ...)to write. IftimerFiredis called from a context that already holds the queue (unlikely but possible in edge cases), this could deadlock.More importantly, at line 199,
[[NSDate date] timeIntervalSince1970]is called outside the sync block after readinglastProgressTimeinside it. This creates a small race window. Consider capturingnowbefore the sync block or inside it for consistency.♻️ Suggested improvement
- (void)logCurrentStats { __block NSUInteger blocks, txs, preFilteredTxs, headers, mnListDiffs, bytes; __block uint32_t chainH, commonH, targetH; __block NSTimeInterval lastProgress; __block double avgKB, totalB; __block NSTimeInterval elapsed; + __block NSTimeInterval now; dispatch_sync(self.statsQueue, ^{ blocks = self.blocksThisInterval; txs = self.transactionsThisInterval; preFilteredTxs = self.preFilteredTxThisInterval; headers = self.headersThisInterval; mnListDiffs = self.mnListDiffsThisInterval; bytes = self.bytesThisInterval; chainH = self.chainHeight; commonH = self.commonHeight; targetH = self.targetHeight; lastProgress = self.lastProgressTime; totalB = (double)self.totalBytes; - elapsed = [[NSDate date] timeIntervalSince1970] - self.startTime; + now = [[NSDate date] timeIntervalSince1970]; + elapsed = now - self.startTime; }); // Calculate KB/sec double lastKB = (double)bytes / 1024.0; double avgKBPerSec = (elapsed > 0) ? (totalB / 1024.0 / elapsed) : 0; // Stall detection - NSTimeInterval now = [[NSDate date] timeIntervalSince1970]; BOOL isStalled = (now - lastProgress) > kStallDetectionSeconds;DashSync/shared/Models/Managers/Chain Managers/DSChainManager.m (1)
240-242: Empty delegate method.
chainWillStartConnectingToPeers:is now empty. If it's required by a protocol, this is fine. Otherwise, consider removing it or adding a comment explaining why it's intentionally empty.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
DashSync/shared/Categories/NSManagedObjectContext+DSSugar.mDashSync/shared/Libraries/DSLogger.hDashSync/shared/Libraries/DSLogger.mDashSync/shared/Models/Derivation Paths/DSFundsDerivationPath.mDashSync/shared/Models/Managers/Chain Managers/DSChainManager.mDashSync/shared/Models/Managers/Chain Managers/DSChainSyncSpeedCalculator.hDashSync/shared/Models/Managers/Chain Managers/DSChainSyncSpeedCalculator.mDashSync/shared/Models/Managers/Chain Managers/DSPeerManager.mDashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.mDashSync/shared/Models/Network/DSPeer.mDashSync/shared/Models/Wallet/DSAccount.m
🚧 Files skipped from review as they are similar to previous changes (2)
- DashSync/shared/Models/Managers/Chain Managers/DSPeerManager.m
- DashSync/shared/Models/Wallet/DSAccount.m
🧰 Additional context used
🪛 Clang (14.0.6)
DashSync/shared/Models/Managers/Chain Managers/DSChainSyncSpeedCalculator.h
[error] 9-9: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Objective-C Linter
- GitHub Check: Testnet Chain
- GitHub Check: Mainnet Chain
- GitHub Check: Test on iPhone simulator
🔇 Additional comments (17)
DashSync/shared/Libraries/DSLogger.h (2)
65-82: Clear API documentation for new logging methods.The new public APIs are well-documented. The
logVersionInfomethod being called automatically during initialization is clearly noted.
28-46: Well-structured Android-style logging macros with good separation of concerns.The logging macros are well-designed with consistent formatting. The
DSLogVerbosemacro being compiled out in release builds is a good practice for performance.Regarding
DSCurrentThreadName(): The implementation is reasonably efficient for typical use cases. It fast-paths main thread detection and named threads (property access only), which covers most logging scenarios. However, for unnamed background threads, it falls back to parsingthread.descriptionvia string operations (rangeOfString, substring extraction), which could have a measurable impact if logging frequently from unnamed threads. Consider whether this is a concern for your logging patterns—if high-frequency logging occurs on unnamed threads, caching or a lighter fallback could help.DashSync/shared/Models/Network/DSPeer.m (3)
219-219: LGTM - Appropriate logging for peer connection initiation.Connection attempts are logged at INFO level, which is appropriate for tracking peer lifecycle events.
306-312: Good differentiation between error and normal disconnects.Using
DSLogWarnfor error disconnections andDSLogInfofor normal disconnections provides appropriate log level separation for troubleshooting.
354-355: Comprehensive connection success logging.The log includes essential peer information (host, port, version, services, useragent) which is valuable for debugging network issues.
DashSync/shared/Models/Managers/Chain Managers/DSTransactionManager.m (6)
165-165: Useful logging for transaction publish list tracking.This helps trace transaction lifecycle from submission through network propagation.
178-180: Good broadcast initiation logging.Includes peer count and transaction hash which are essential for debugging broadcast issues.
1192-1194: Speed calculator integration looks correct.The transaction recording is called after amount calculation and logging, which is appropriate.
1445-1459: Comprehensive InstantSend lock logging.Logs reception, existing lock detection, and verification status - all essential for debugging IS issues.
1627-1631: ChainLock logging mirrors InstantSend pattern.Consistent logging approach for ChainLock reception and verification.
1540-1545: Block sync tracking with speed calculator.The integration records block reception and updates chain height tracking. The
commonHeightparameter is intentionally set equal tolastSyncBlockHeightfor progress logging purposes.DashSync/shared/Libraries/DSLogger.m (3)
52-65: StaticNSDateFormatteris thread-safe for read-only operations.The
dispatch_onceinitialization and read-only usage of the formatter is correct and thread-safe. The UTC timezone andHH:mm:ssformat match the Android-style logging requirements per the PR objectives.
135-141: Publiclog:method bypasses the structured format.
+log:usesDDLogInfodirectly, which won't include the[thread] ClassName -prefix that the macros add. This is inconsistent with+log:className:which usesDSLogInfo. Consider if this is intentional for raw message logging or if it should use a consistent format.
143-159: Version info logging implementation looks good.The startup logging correctly retrieves app and framework version info with appropriate fallbacks. The format matches the Android-style output described in the PR objectives.
DashSync/shared/Models/Managers/Chain Managers/DSChainSyncSpeedCalculator.h (1)
22-68: Well-designed public API for sync telemetry.The header provides a clean, comprehensive interface for tracking sync statistics. The singleton pattern is appropriate for a global metrics collector. The documentation comments clearly explain the purpose of each method.
Note: The static analysis error about
Foundation/Foundation.hnot found is a false positive - this is standard iOS/macOS framework header that will be available at build time.DashSync/shared/Models/Managers/Chain Managers/DSChainManager.m (2)
129-135: Sync calculator integration looks correct.The reset and start sequence properly initializes the speed calculator before connecting to peers. The logging provides good visibility into sync lifecycle.
Note: As mentioned in the DSChainSyncSpeedCalculator review, the
resetmethod usesdispatch_async, so there's a theoretical race between reset completing andstartCalculatingbeginning. If the suggested fix to usedispatch_syncinresetis applied, this integration will be fully correct.
137-144: Stop sync correctly stops the speed calculator.The integration properly stops telemetry collection when sync is stopped.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| - (NSError *)ds_save { | ||
| if (!self.hasChanges) return nil; | ||
|
|
||
| NSTimeInterval saveStart = [[NSDate date] timeIntervalSince1970]; | ||
| NSUInteger insertedCount = self.insertedObjects.count; | ||
| NSUInteger updatedCount = self.updatedObjects.count; | ||
| NSUInteger deletedCount = self.deletedObjects.count; | ||
|
|
||
| #if TARGET_OS_IOS | ||
| NSUInteger taskId = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{ | ||
| }]; | ||
| #endif | ||
| NSError *error = nil; | ||
| if (![self save:&error]) { // persist changes | ||
| DSLog(@"%s: %@", __func__, error); | ||
| #if DEBUG | ||
| abort(); | ||
| #endif | ||
| } | ||
| #if TARGET_OS_IOS | ||
| [[UIApplication sharedApplication] endBackgroundTask:taskId]; | ||
| #endif | ||
|
|
||
| NSTimeInterval saveTime = ([[NSDate date] timeIntervalSince1970] - saveStart) * 1000.0; | ||
| DSLogInfo(@"CoreData", @"Save completed in %.1f ms (inserted: %lu, updated: %lu, deleted: %lu)", | ||
| saveTime, (unsigned long)insertedCount, (unsigned long)updatedCount, (unsigned long)deletedCount); | ||
|
|
||
| return error; |
There was a problem hiding this comment.
Logging occurs even on save failure.
The timing and counts are captured before the save, which is correct since Core Data clears these sets after save. However, the log at lines 91-92 will report "Save completed" even if the save failed (in release builds where abort is not called).
🐛 Proposed fix to only log on success
NSError *error = nil;
if (![self save:&error]) { // persist changes
`#if` DEBUG
abort();
`#endif`
+ return error;
}
`#if` TARGET_OS_IOS
[[UIApplication sharedApplication] endBackgroundTask:taskId];
`#endif`
NSTimeInterval saveTime = ([[NSDate date] timeIntervalSince1970] - saveStart) * 1000.0;
DSLogInfo(@"CoreData", @"Save completed in %.1f ms (inserted: %lu, updated: %lu, deleted: %lu)",
saveTime, (unsigned long)insertedCount, (unsigned long)updatedCount, (unsigned long)deletedCount);
return error;📝 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.
| - (NSError *)ds_save { | |
| if (!self.hasChanges) return nil; | |
| NSTimeInterval saveStart = [[NSDate date] timeIntervalSince1970]; | |
| NSUInteger insertedCount = self.insertedObjects.count; | |
| NSUInteger updatedCount = self.updatedObjects.count; | |
| NSUInteger deletedCount = self.deletedObjects.count; | |
| #if TARGET_OS_IOS | |
| NSUInteger taskId = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{ | |
| }]; | |
| #endif | |
| NSError *error = nil; | |
| if (![self save:&error]) { // persist changes | |
| DSLog(@"%s: %@", __func__, error); | |
| #if DEBUG | |
| abort(); | |
| #endif | |
| } | |
| #if TARGET_OS_IOS | |
| [[UIApplication sharedApplication] endBackgroundTask:taskId]; | |
| #endif | |
| NSTimeInterval saveTime = ([[NSDate date] timeIntervalSince1970] - saveStart) * 1000.0; | |
| DSLogInfo(@"CoreData", @"Save completed in %.1f ms (inserted: %lu, updated: %lu, deleted: %lu)", | |
| saveTime, (unsigned long)insertedCount, (unsigned long)updatedCount, (unsigned long)deletedCount); | |
| return error; | |
| - (NSError *)ds_save { | |
| if (!self.hasChanges) return nil; | |
| NSTimeInterval saveStart = [[NSDate date] timeIntervalSince1970]; | |
| NSUInteger insertedCount = self.insertedObjects.count; | |
| NSUInteger updatedCount = self.updatedObjects.count; | |
| NSUInteger deletedCount = self.deletedObjects.count; | |
| `#if` TARGET_OS_IOS | |
| NSUInteger taskId = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{ | |
| }]; | |
| `#endif` | |
| NSError *error = nil; | |
| if (![self save:&error]) { // persist changes | |
| `#if` DEBUG | |
| abort(); | |
| `#endif` | |
| } | |
| `#if` TARGET_OS_IOS | |
| [[UIApplication sharedApplication] endBackgroundTask:taskId]; | |
| `#endif` | |
| if (error) return error; | |
| NSTimeInterval saveTime = ([[NSDate date] timeIntervalSince1970] - saveStart) * 1000.0; | |
| DSLogInfo(@"CoreData", @"Save completed in %.1f ms (inserted: %lu, updated: %lu, deleted: %lu)", | |
| saveTime, (unsigned long)insertedCount, (unsigned long)updatedCount, (unsigned long)deletedCount); | |
| return error; | |
| } |
🤖 Prompt for AI Agents
In `@DashSync/shared/Categories/NSManagedObjectContext`+DSSugar.m around lines 68
- 94, ds_save currently logs "Save completed" and timing/counts unconditionally
even when the save (the -[NSManagedObjectContext save:&error] call in ds_save)
fails; modify ds_save so that the DSLogInfo call (and the computed
saveTime/insertedCount/updatedCount/deletedCount logging) occurs only when
save:&error returns YES (i.e., after a successful save), while still ensuring
the background task is ended and the NSError is returned on failure; locate the
save call and DSLogInfo in ds_save and wrap the logging in the success branch so
failures do not produce a "Save completed" message.
| NSUInteger keysNeeded = gapLimit - a.count; | ||
| if (keysNeeded > 0) { | ||
| NSTimeInterval keyGenStart = [[NSDate date] timeIntervalSince1970]; | ||
| NSUInteger numChildren = [(internal) ? self.internalAddresses : self.externalAddresses count]; | ||
| DSLogInfo(@"DSFundsDerivationPath", @"%lu keys needed for %@ = %lu issued + %lu lookahead size + %lu lookahead threshold - %lu num children", | ||
| (unsigned long)keysNeeded, self.stringRepresentation, | ||
| (unsigned long)(n - keysNeeded), (unsigned long)gapLimit, (unsigned long)0, | ||
| (unsigned long)numChildren); | ||
|
|
||
| while (a.count < gapLimit) { // generate new addresses up to gapLimit | ||
| NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal]; | ||
| NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType]; | ||
|
|
||
| if (!addr) { | ||
| if (error) { | ||
| *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"]; | ||
| } | ||
| return nil; | ||
| } | ||
| return nil; | ||
|
|
||
| [self.mAllAddresses addObject:addr]; | ||
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | ||
| [a addObject:addr]; | ||
| [addAddresses setObject:addr forKey:@(n)]; | ||
| n++; | ||
| } | ||
|
|
||
| [self.mAllAddresses addObject:addr]; | ||
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | ||
| [a addObject:addr]; | ||
| [addAddresses setObject:addr forKey:@(n)]; | ||
| n++; | ||
| NSTimeInterval keyGenTime = ([[NSDate date] timeIntervalSince1970] - keyGenStart) * 1000.0; | ||
| DSLogInfo(@"DSFundsDerivationPath", @"Took %.2f ms", keyGenTime); | ||
| } else { | ||
| while (a.count < gapLimit) { // generate new addresses up to gapLimit | ||
| NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal]; | ||
| NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType]; | ||
|
|
||
| if (!addr) { | ||
| if (error) { | ||
| *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"]; | ||
| } | ||
| return nil; | ||
| } | ||
|
|
||
| [self.mAllAddresses addObject:addr]; | ||
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | ||
| [a addObject:addr]; | ||
| [addAddresses setObject:addr forKey:@(n)]; | ||
| n++; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The else branch (lines 213-231) appears to be dead code.
The condition keysNeeded > 0 at line 185 is always true when this code is reached because:
- Line 180 returns early if
a.count >= gapLimit keysNeeded = gapLimit - a.count(line 184)- If we reach line 185, then
a.count < gapLimit, which meanskeysNeeded > 0
This makes the else branch unreachable. The duplicate key generation code should be consolidated.
♻️ Proposed refactor to remove dead code
NSMutableDictionary *addAddresses = [NSMutableDictionary dictionary];
NSUInteger keysNeeded = gapLimit - a.count;
- if (keysNeeded > 0) {
- NSTimeInterval keyGenStart = [[NSDate date] timeIntervalSince1970];
- NSUInteger numChildren = [(internal) ? self.internalAddresses : self.externalAddresses count];
- DSLogInfo(@"DSFundsDerivationPath", @"%lu keys needed for %@ = %lu issued + %lu lookahead size + %lu lookahead threshold - %lu num children",
- (unsigned long)keysNeeded, self.stringRepresentation,
- (unsigned long)(n - keysNeeded), (unsigned long)gapLimit, (unsigned long)0,
- (unsigned long)numChildren);
-
- while (a.count < gapLimit) { // generate new addresses up to gapLimit
- NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal];
- NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType];
-
- if (!addr) {
- if (error) {
- *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"];
- }
- return nil;
- }
-
- [self.mAllAddresses addObject:addr];
- [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr];
- [a addObject:addr];
- [addAddresses setObject:addr forKey:@(n)];
- n++;
+ NSTimeInterval keyGenStart = [[NSDate date] timeIntervalSince1970];
+ NSUInteger numChildren = [(internal) ? self.internalAddresses : self.externalAddresses count];
+ DSLogInfo(@"DSFundsDerivationPath", @"%lu keys needed for %@ = %lu issued + %lu lookahead size + %lu lookahead threshold - %lu num children",
+ (unsigned long)keysNeeded, self.stringRepresentation,
+ (unsigned long)(n - keysNeeded), (unsigned long)gapLimit, (unsigned long)0,
+ (unsigned long)numChildren);
+
+ while (a.count < gapLimit) { // generate new addresses up to gapLimit
+ NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal];
+ NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType];
+
+ if (!addr) {
+ if (error) {
+ *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"];
+ }
+ return nil;
}
- NSTimeInterval keyGenTime = ([[NSDate date] timeIntervalSince1970] - keyGenStart) * 1000.0;
- DSLogInfo(@"DSFundsDerivationPath", @"Took %.2f ms", keyGenTime);
- } else {
- while (a.count < gapLimit) { // generate new addresses up to gapLimit
- NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal];
- NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType];
-
- if (!addr) {
- if (error) {
- *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"];
- }
- return nil;
- }
-
- [self.mAllAddresses addObject:addr];
- [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr];
- [a addObject:addr];
- [addAddresses setObject:addr forKey:@(n)];
- n++;
- }
+ [self.mAllAddresses addObject:addr];
+ [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr];
+ [a addObject:addr];
+ [addAddresses setObject:addr forKey:@(n)];
+ n++;
}
+
+ NSTimeInterval keyGenTime = ([[NSDate date] timeIntervalSince1970] - keyGenStart) * 1000.0;
+ DSLogInfo(@"DSFundsDerivationPath", @"Took %.2f ms", keyGenTime);📝 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.
| NSUInteger keysNeeded = gapLimit - a.count; | |
| if (keysNeeded > 0) { | |
| NSTimeInterval keyGenStart = [[NSDate date] timeIntervalSince1970]; | |
| NSUInteger numChildren = [(internal) ? self.internalAddresses : self.externalAddresses count]; | |
| DSLogInfo(@"DSFundsDerivationPath", @"%lu keys needed for %@ = %lu issued + %lu lookahead size + %lu lookahead threshold - %lu num children", | |
| (unsigned long)keysNeeded, self.stringRepresentation, | |
| (unsigned long)(n - keysNeeded), (unsigned long)gapLimit, (unsigned long)0, | |
| (unsigned long)numChildren); | |
| while (a.count < gapLimit) { // generate new addresses up to gapLimit | |
| NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal]; | |
| NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType]; | |
| if (!addr) { | |
| if (error) { | |
| *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"]; | |
| } | |
| return nil; | |
| } | |
| return nil; | |
| [self.mAllAddresses addObject:addr]; | |
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | |
| [a addObject:addr]; | |
| [addAddresses setObject:addr forKey:@(n)]; | |
| n++; | |
| } | |
| [self.mAllAddresses addObject:addr]; | |
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | |
| [a addObject:addr]; | |
| [addAddresses setObject:addr forKey:@(n)]; | |
| n++; | |
| NSTimeInterval keyGenTime = ([[NSDate date] timeIntervalSince1970] - keyGenStart) * 1000.0; | |
| DSLogInfo(@"DSFundsDerivationPath", @"Took %.2f ms", keyGenTime); | |
| } else { | |
| while (a.count < gapLimit) { // generate new addresses up to gapLimit | |
| NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal]; | |
| NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType]; | |
| if (!addr) { | |
| if (error) { | |
| *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"]; | |
| } | |
| return nil; | |
| } | |
| [self.mAllAddresses addObject:addr]; | |
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | |
| [a addObject:addr]; | |
| [addAddresses setObject:addr forKey:@(n)]; | |
| n++; | |
| } | |
| } | |
| NSMutableDictionary *addAddresses = [NSMutableDictionary dictionary]; | |
| NSUInteger keysNeeded = gapLimit - a.count; | |
| NSTimeInterval keyGenStart = [[NSDate date] timeIntervalSince1970]; | |
| NSUInteger numChildren = [(internal) ? self.internalAddresses : self.externalAddresses count]; | |
| DSLogInfo(@"DSFundsDerivationPath", @"%lu keys needed for %@ = %lu issued + %lu lookahead size + %lu lookahead threshold - %lu num children", | |
| (unsigned long)keysNeeded, self.stringRepresentation, | |
| (unsigned long)(n - keysNeeded), (unsigned long)gapLimit, (unsigned long)0, | |
| (unsigned long)numChildren); | |
| while (a.count < gapLimit) { // generate new addresses up to gapLimit | |
| NSData *pubKey = [self publicKeyDataAtIndex:n internal:internal]; | |
| NSString *addr = [DSKeyManager ecdsaKeyAddressFromPublicKeyData:pubKey forChainType:self.chain.chainType]; | |
| if (!addr) { | |
| if (error) { | |
| *error = [NSError errorWithCode:500 localizedDescriptionKey:@"Error generating public keys"]; | |
| } | |
| return nil; | |
| } | |
| [self.mAllAddresses addObject:addr]; | |
| [(internal) ? self.internalAddresses : self.externalAddresses addObject:addr]; | |
| [a addObject:addr]; | |
| [addAddresses setObject:addr forKey:@(n)]; | |
| n++; | |
| } | |
| NSTimeInterval keyGenTime = ([[NSDate date] timeIntervalSince1970] - keyGenStart) * 1000.0; | |
| DSLogInfo(@"DSFundsDerivationPath", @"Took %.2f ms", keyGenTime); |
🤖 Prompt for AI Agents
In `@DashSync/shared/Models/Derivation` Paths/DSFundsDerivationPath.m around lines
184 - 231, The else branch is unreachable because keysNeeded = gapLimit -
a.count is always > 0 when we get past the earlier return, so duplicate
address-generation logic should be consolidated: remove the entire else branch
(lines with the second while block) and keep a single address-generation loop
that uses the existing logic (call to publicKeyDataAtIndex:internal:,
DSKeyManager ecdsaKeyAddressFromPublicKeyData:forChainType:, checks for nil
error, adding to mAllAddresses and either internalAddresses or
externalAddresses, appending to a and addAddresses, and incrementing n); retain
the DSLogInfo timing/logging code only when keysNeeded > 0 (i.e., compute
keysNeeded, conditionally log, then run the single shared while (a.count <
gapLimit) loop).
| // Log transaction list after sync completion | ||
| for (DSWallet *wallet in chain.wallets) { | ||
| NSArray *transactions = wallet.allTransactions; | ||
| DSLogInfo(@"DSChainManager", @"Wallet has %lu transactions after sync", (unsigned long)transactions.count); | ||
| for (DSTransaction *tx in transactions) { | ||
| NSString *status = tx.blockHeight == TX_UNCONFIRMED ? @"unconfirmed" : [NSString stringWithFormat:@"confirmed at %u", tx.blockHeight]; | ||
| DSLogDebug(@"DSChainManager", @" TX %@ - %@", uint256_reverse_hex(tx.txHash), status); | ||
| } | ||
| } |
There was a problem hiding this comment.
Transaction logging after sync may impact performance and expose sensitive data.
Iterating all wallet transactions and logging each one after sync could:
- Performance: Cause delays on wallets with many transactions
- Privacy: Transaction hashes in logs could be sensitive
Consider either:
- Guarding with a debug/verbose flag
- Limiting to a sample (e.g., last 10 transactions)
- Using
DSLogVerboseinstead ofDSLogDebugfor per-transaction details
♻️ Suggested improvement
// Log transaction list after sync completion
for (DSWallet *wallet in chain.wallets) {
NSArray *transactions = wallet.allTransactions;
DSLogInfo(@"DSChainManager", @"Wallet has %lu transactions after sync", (unsigned long)transactions.count);
- for (DSTransaction *tx in transactions) {
- NSString *status = tx.blockHeight == TX_UNCONFIRMED ? @"unconfirmed" : [NSString stringWithFormat:@"confirmed at %u", tx.blockHeight];
- DSLogDebug(@"DSChainManager", @" TX %@ - %@", uint256_reverse_hex(tx.txHash), status);
- }
+ // Log only unconfirmed transactions for debugging
+ NSUInteger unconfirmedCount = 0;
+ for (DSTransaction *tx in transactions) {
+ if (tx.blockHeight == TX_UNCONFIRMED) {
+ unconfirmedCount++;
+ DSLogVerbose(@"DSChainManager", @" Unconfirmed TX %@", uint256_reverse_hex(tx.txHash));
+ }
+ }
+ if (unconfirmedCount > 0) {
+ DSLogDebug(@"DSChainManager", @" %lu unconfirmed transactions", (unsigned long)unconfirmedCount);
+ }
}📝 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.
| // Log transaction list after sync completion | |
| for (DSWallet *wallet in chain.wallets) { | |
| NSArray *transactions = wallet.allTransactions; | |
| DSLogInfo(@"DSChainManager", @"Wallet has %lu transactions after sync", (unsigned long)transactions.count); | |
| for (DSTransaction *tx in transactions) { | |
| NSString *status = tx.blockHeight == TX_UNCONFIRMED ? @"unconfirmed" : [NSString stringWithFormat:@"confirmed at %u", tx.blockHeight]; | |
| DSLogDebug(@"DSChainManager", @" TX %@ - %@", uint256_reverse_hex(tx.txHash), status); | |
| } | |
| } | |
| // Log transaction list after sync completion | |
| for (DSWallet *wallet in chain.wallets) { | |
| NSArray *transactions = wallet.allTransactions; | |
| DSLogInfo(@"DSChainManager", @"Wallet has %lu transactions after sync", (unsigned long)transactions.count); | |
| // Log only unconfirmed transactions for debugging | |
| NSUInteger unconfirmedCount = 0; | |
| for (DSTransaction *tx in transactions) { | |
| if (tx.blockHeight == TX_UNCONFIRMED) { | |
| unconfirmedCount++; | |
| DSLogVerbose(@"DSChainManager", @" Unconfirmed TX %@", uint256_reverse_hex(tx.txHash)); | |
| } | |
| } | |
| if (unconfirmedCount > 0) { | |
| DSLogDebug(@"DSChainManager", @" %lu unconfirmed transactions", (unsigned long)unconfirmedCount); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@DashSync/shared/Models/Managers/Chain` Managers/DSChainManager.m around lines
287 - 295, The per-transaction logging in DSChainManager iterates
wallet.allTransactions and logs every DSTransaction (using uint256_reverse_hex
and DSLogDebug), which can hurt performance and leak sensitive data; modify the
block so that DSLogInfo still reports the total count but per-transaction
details are either gated behind a debug/verbose flag (check a global
verbose/debug setting) or limited to a sample (e.g., only log the last 10
transactions), and change DSLogDebug to DSLogVerbose for those detailed lines;
ensure you still preserve the TX_UNCONFIRMED check and message formatting when
emitting the gated/limited logs.
| - (void)startCalculating { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [self.logTimer invalidate]; | ||
| self.startTime = [[NSDate date] timeIntervalSince1970]; | ||
| self.lastProgressTime = self.startTime; | ||
| self.logTimer = [NSTimer scheduledTimerWithTimeInterval:kLogIntervalSeconds | ||
| target:self | ||
| selector:@selector(timerFired) | ||
| userInfo:nil | ||
| repeats:YES]; | ||
| [[NSRunLoop currentRunLoop] addTimer:self.logTimer forMode:NSRunLoopCommonModes]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Timer added to run loop twice.
scheduledTimerWithTimeInterval: already adds the timer to the current run loop in the default mode. The subsequent addTimer:forMode: call adds it again for NSRunLoopCommonModes. This double-addition is likely unintentional. If you want common modes support, use the non-scheduling initializer instead.
🐛 Proposed fix
- (void)startCalculating {
dispatch_async(dispatch_get_main_queue(), ^{
[self.logTimer invalidate];
self.startTime = [[NSDate date] timeIntervalSince1970];
self.lastProgressTime = self.startTime;
- self.logTimer = [NSTimer scheduledTimerWithTimeInterval:kLogIntervalSeconds
- target:self
- selector:`@selector`(timerFired)
- userInfo:nil
- repeats:YES];
- [[NSRunLoop currentRunLoop] addTimer:self.logTimer forMode:NSRunLoopCommonModes];
+ self.logTimer = [NSTimer timerWithTimeInterval:kLogIntervalSeconds
+ target:self
+ selector:`@selector`(timerFired)
+ userInfo:nil
+ repeats:YES];
+ [[NSRunLoop currentRunLoop] addTimer:self.logTimer forMode:NSRunLoopCommonModes];
});
}📝 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.
| - (void)startCalculating { | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [self.logTimer invalidate]; | |
| self.startTime = [[NSDate date] timeIntervalSince1970]; | |
| self.lastProgressTime = self.startTime; | |
| self.logTimer = [NSTimer scheduledTimerWithTimeInterval:kLogIntervalSeconds | |
| target:self | |
| selector:@selector(timerFired) | |
| userInfo:nil | |
| repeats:YES]; | |
| [[NSRunLoop currentRunLoop] addTimer:self.logTimer forMode:NSRunLoopCommonModes]; | |
| }); | |
| } | |
| - (void)startCalculating { | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [self.logTimer invalidate]; | |
| self.startTime = [[NSDate date] timeIntervalSince1970]; | |
| self.lastProgressTime = self.startTime; | |
| self.logTimer = [NSTimer timerWithTimeInterval:kLogIntervalSeconds | |
| target:self | |
| selector:`@selector`(timerFired) | |
| userInfo:nil | |
| repeats:YES]; | |
| [[NSRunLoop currentRunLoop] addTimer:self.logTimer forMode:NSRunLoopCommonModes]; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@DashSync/shared/Models/Managers/Chain` Managers/DSChainSyncSpeedCalculator.m
around lines 75 - 87, In startCalculating, avoid adding the same NSTimer twice:
replace the scheduledTimerWithTimeInterval: call with the non-scheduling
initializer timerWithTimeInterval:target:selector:userInfo:repeats: and then add
that timer to the run loop with addTimer:forMode: (so the timer is created
without being auto-scheduled and then registered for NSRunLoopCommonModes); keep
the existing invalidate of self.logTimer and assignments to
self.startTime/self.lastProgressTime and ensure the created timer is stored to
self.logTimer and uses the selector timerFired.
| - (void)resetIntervalCounters { | ||
| dispatch_async(self.statsQueue, ^{ | ||
| self.blocksThisInterval = 0; | ||
| self.transactionsThisInterval = 0; | ||
| self.preFilteredTxThisInterval = 0; | ||
| self.headersThisInterval = 0; | ||
| self.mnListDiffsThisInterval = 0; | ||
| self.bytesThisInterval = 0; | ||
| }); | ||
| } | ||
|
|
||
| - (void)reset { | ||
| dispatch_async(self.statsQueue, ^{ | ||
| self.blocksThisInterval = 0; | ||
| self.transactionsThisInterval = 0; | ||
| self.preFilteredTxThisInterval = 0; | ||
| self.headersThisInterval = 0; | ||
| self.mnListDiffsThisInterval = 0; | ||
| self.bytesThisInterval = 0; | ||
| self.totalBlocks = 0; | ||
| self.totalTransactions = 0; | ||
| self.totalBytes = 0; | ||
| self.startTime = [[NSDate date] timeIntervalSince1970]; | ||
| self.avgKBPerSec = 0; | ||
| self.lastKBPerSec = 0; | ||
| self.chainHeight = 0; | ||
| self.commonHeight = 0; | ||
| self.targetHeight = 0; | ||
| self.lastProgressTime = self.startTime; | ||
| self.lastChainHeight = 0; | ||
| [self.historyBuffer removeAllObjects]; | ||
| }); | ||
| } |
There was a problem hiding this comment.
reset method should use dispatch_sync for immediate effect.
Both resetIntervalCounters and reset use dispatch_async, meaning callers cannot rely on the reset being complete when the method returns. This could cause issues if startCalculating is called immediately after reset (as done in DSChainManager.startSync), since stale values might be read before the async reset completes.
🐛 Proposed fix for `reset`
- (void)reset {
- dispatch_async(self.statsQueue, ^{
+ dispatch_sync(self.statsQueue, ^{
self.blocksThisInterval = 0;
self.transactionsThisInterval = 0;
// ... rest of reset logic
});
}📝 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.
| - (void)resetIntervalCounters { | |
| dispatch_async(self.statsQueue, ^{ | |
| self.blocksThisInterval = 0; | |
| self.transactionsThisInterval = 0; | |
| self.preFilteredTxThisInterval = 0; | |
| self.headersThisInterval = 0; | |
| self.mnListDiffsThisInterval = 0; | |
| self.bytesThisInterval = 0; | |
| }); | |
| } | |
| - (void)reset { | |
| dispatch_async(self.statsQueue, ^{ | |
| self.blocksThisInterval = 0; | |
| self.transactionsThisInterval = 0; | |
| self.preFilteredTxThisInterval = 0; | |
| self.headersThisInterval = 0; | |
| self.mnListDiffsThisInterval = 0; | |
| self.bytesThisInterval = 0; | |
| self.totalBlocks = 0; | |
| self.totalTransactions = 0; | |
| self.totalBytes = 0; | |
| self.startTime = [[NSDate date] timeIntervalSince1970]; | |
| self.avgKBPerSec = 0; | |
| self.lastKBPerSec = 0; | |
| self.chainHeight = 0; | |
| self.commonHeight = 0; | |
| self.targetHeight = 0; | |
| self.lastProgressTime = self.startTime; | |
| self.lastChainHeight = 0; | |
| [self.historyBuffer removeAllObjects]; | |
| }); | |
| } | |
| - (void)resetIntervalCounters { | |
| dispatch_async(self.statsQueue, ^{ | |
| self.blocksThisInterval = 0; | |
| self.transactionsThisInterval = 0; | |
| self.preFilteredTxThisInterval = 0; | |
| self.headersThisInterval = 0; | |
| self.mnListDiffsThisInterval = 0; | |
| self.bytesThisInterval = 0; | |
| }); | |
| } | |
| - (void)reset { | |
| dispatch_sync(self.statsQueue, ^{ | |
| self.blocksThisInterval = 0; | |
| self.transactionsThisInterval = 0; | |
| self.preFilteredTxThisInterval = 0; | |
| self.headersThisInterval = 0; | |
| self.mnListDiffsThisInterval = 0; | |
| self.bytesThisInterval = 0; | |
| self.totalBlocks = 0; | |
| self.totalTransactions = 0; | |
| self.totalBytes = 0; | |
| self.startTime = [[NSDate date] timeIntervalSince1970]; | |
| self.avgKBPerSec = 0; | |
| self.lastKBPerSec = 0; | |
| self.chainHeight = 0; | |
| self.commonHeight = 0; | |
| self.targetHeight = 0; | |
| self.lastProgressTime = self.startTime; | |
| self.lastChainHeight = 0; | |
| [self.historyBuffer removeAllObjects]; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@DashSync/shared/Models/Managers/Chain` Managers/DSChainSyncSpeedCalculator.m
around lines 259 - 291, reset currently uses dispatch_async on self.statsQueue
which defers clearing state and can leave stale values; change the
implementation of - (void)reset to use dispatch_sync(self.statsQueue, ^{ ... })
instead of dispatch_async so the reset completes before the method returns;
leave resetIntervalCounters as-is (still using dispatch_async) and ensure you
only modify the dispatch call in the -reset method that sets blocksThisInterval,
transactionsThisInterval, preFilteredTxThisInterval, headersThisInterval,
mnListDiffsThisInterval, bytesThisInterval, totalBlocks, totalTransactions,
totalBytes, startTime, avgKBPerSec, lastKBPerSec, chainHeight, commonHeight,
targetHeight, lastProgressTime, lastChainHeight and historyBuffer removal.
- Convert satoshi values to DASH (divide by 100,000,000) - Skip logging initial UINT64_MAX state to avoid confusing output - Format all amounts with 8 decimal places for consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
DSLog/DSLogPrivatestatements from codebase (70+ files)Changes
New Logging Infrastructure
DSLogInfo,DSLogDebug,DSLogWarn,DSLogError,DSLogVerbosemacrosDSCurrentThreadName()helper for thread identificationDSAndroidStyleLogFormatterforHH:mm:sstimestamp formatLogging Coverage
Log Format
Example:
Log Compaction
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.