fix(coinjoin): crashes #591
Conversation
WalkthroughThis pull request refactors multiple components across the codebase. In the coin join wrapper, it removes synchronization from certain methods and adds two new class methods to determine transaction types. In the key manager, it introduces input validation for public key data while removing an obsolete method. Additionally, the asset lock and unlock transaction classes have updated their data serialization methods to include an extra boolean parameter ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant W as DSCoinJoinWrapper
C->>W: coinJoinTxTypeForTransaction:account:(transaction, account)
W-->>C: Return CoinJoinTransactionType
sequenceDiagram
participant C as Caller
participant T as DSAssetLock/UnlockTransaction
participant S as Super Transaction Handler
C->>T: toDataWithSubscriptIndex: anyoneCanPay:
T->>S: Forward call with provided anyoneCanPay flag
S-->>T: Return serialized data
T-->>C: Return data blob
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
DashSync/shared/Models/Managers/Chain Managers/DSKeyManager.m (1)
202-205: 💡 Verification agent🧩 Analysis chain
Verify the removal of this method.
The method
ecdsaKeyPublicKeyUniqueIDFromDerivedKeyData:forChainType:has been removed. Please ensure there are no remaining references to this method in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to the removed method. # Test: Search for method references. Expect: No matches. rg -l "ecdsaKeyPublicKeyUniqueIDFromDerivedKeyData"Length of output: 175
Action: Incomplete Method Removal – Update DSKeyManager Files
The removal of
ecdsaKeyPublicKeyUniqueIDFromDerivedKeyData:forChainType:hasn't been fully applied. Our search shows references still exist in:
- DSKeyManager.h – Likely still contains the method declaration.
- DSKeyManager.m – Still includes the method implementation.
Please ensure that both the declaration in the header and the implementation in the source file are removed and that no other parts of the codebase reference this method. Once these updates are made, verify again with a similar search to confirm complete removal.
🧹 Nitpick comments (2)
DashSync/shared/Models/Transactions/Base/DSAssetLockTransaction.m (1)
80-87: LGTM! Consider adding documentation for the new parameter.The method signature change and implementation look correct. The
anyoneCanPayparameter is properly propagated to the superclass while maintaining thread safety with@synchronized.Consider adding documentation to explain the purpose and impact of the
anyoneCanPayparameter, as it affects transaction signing behavior. For example:/** * Serializes the transaction for signing. * @param subscriptIndex The index of the input being signed, or NSNotFound if not for signing * @param anyoneCanPay When YES, indicates that the signature will only cover the current input, * allowing other inputs to be added later * @return The serialized transaction data */ - (NSData *)toDataWithSubscriptIndex:(NSUInteger)subscriptIndex anyoneCanPay:(BOOL)anyoneCanPay;DashSync/shared/Models/Transactions/Base/DSAssetUnlockTransaction.m (1)
84-89: Consider removing unnecessary synchronization.The synchronized block appears unnecessary as the method only performs read operations and creates new data. NSMutableData operations are already thread-safe in this context, and no shared state is being modified.
- @synchronized(self) { NSMutableData *data = [[super toDataWithSubscriptIndex:subscriptIndex anyoneCanPay:anyoneCanPay] mutableCopy]; [data appendCountedData:[self payloadData]]; if (subscriptIndex != NSNotFound) [data appendUInt32:SIGHASH_ALL]; return data; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m(3 hunks)DashSync/shared/Models/Managers/Chain Managers/DSKeyManager.m(1 hunks)DashSync/shared/Models/Transactions/Base/DSAssetLockTransaction.m(1 hunks)DashSync/shared/Models/Transactions/Base/DSAssetUnlockTransaction.m(1 hunks)
🔇 Additional comments (4)
DashSync/shared/Models/Transactions/Base/DSAssetUnlockTransaction.m (1)
83-86: LGTM! Clean implementation of the anyoneCanPay parameter.The new parameter is correctly integrated and properly passed to the superclass method, maintaining consistency with similar changes in the codebase.
DashSync/shared/Models/Managers/Chain Managers/DSKeyManager.m (1)
194-200: LGTM! Input validation prevents potential crashes.The added validation checks for nil or empty data prevent potential crashes by ensuring valid input before proceeding with address generation.
DashSync/shared/Models/CoinJoin/DSCoinJoinWrapper.m (2)
99-101: LGTM! Removed unnecessary synchronization.The removal of
@synchronizedblocks from these methods is appropriate as they only access static/constant values without modifying shared state. This change improves performance by eliminating unnecessary synchronization overhead.Also applies to: 203-205, 207-209, 217-219
157-177: LGTM! Well-implemented transaction type detection.The new methods for determining CoinJoin transaction types:
- Properly handle memory allocation and deallocation
- Follow existing patterns for transaction handling
- Include appropriate error handling
2 participants
What was done?
ecdsaKeyAddressFromPublicKeyDataHow Has This Been Tested?
QA
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Refactor