fix: change Wallet.lock to ReentrantReadWriteLock #283
Conversation
WalkthroughThe changes introduce a new static method in the Threading utility for creating named ReentrantReadWriteLocks. The Wallet class switches its internal synchronization from a simple ReentrantLock to a ReentrantReadWriteLock, updating all usages accordingly. WalletEx updates its read-only methods to use the shared read lock instead of the exclusive write lock. Changes
Estimated code review effort3 (~45 minutes) Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)core/src/main/java/org/bitcoinj/wallet/Wallet.java✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/bitcoinj/utils/Threading.java(2 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(111 hunks)core/src/main/java/org/bitcoinj/wallet/WalletEx.java(16 hunks)
🧬 Code Graph Analysis (1)
core/src/main/java/org/bitcoinj/utils/Threading.java (1)
core/src/main/java/org/bitcoinj/core/Utils.java (1)
Utils(64-862)
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/src/main/java/org/bitcoinj/utils/Threading.java (1)
core/src/main/java/org/bitcoinj/core/Utils.java (1)
Utils(64-862)
🔇 Additional comments (13)
core/src/main/java/org/bitcoinj/utils/Threading.java (2)
34-34: LGTM - Import addition is correct.The import for
ReentrantReadWriteLockis properly added to support the newreadWriteLockmethod.
196-201: LGTM - Consistent implementation pattern.The
readWriteLockmethod correctly follows the same pattern as the existinglockmethod:
- Uses the same Android runtime detection and policy checks
- Returns a fair ReentrantReadWriteLock for Android environments
- Delegates to the cycle-detecting factory for other environments
This ensures consistent behavior and proper cycle detection capabilities.
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (9)
202-202: LGTM - Correct use of read lock for balance calculation.The
getBalancemethod is a read-only operation that calculates wallet balance without modifying state. UsingreadLock()is appropriate and allows concurrent balance queries from multiple threads.Also applies to: 237-237
285-285: LGTM - Appropriate read lock for spent status check.The
isSpentmethod only reads transaction state and doesn't modify the wallet. Using the read lock is correct and improves concurrency.Also applies to: 295-295
318-318: LGTM - Read lock correctly used for ownership check.The
isMine(TransactionInput input)method only reads transaction data to determine ownership. The read lock usage is appropriate for this read-only operation.Also applies to: 327-327
333-333: LGTM - Read lock appropriate for CoinJoin rounds calculation.The
getRealOutpointCoinJoinRoundsmethod performs complex read-only analysis of transaction chains. Using the read lock allows multiple threads to perform this calculation concurrently without blocking each other.Also applies to: 422-422
434-434: LGTM - Read lock suitable for fully mixed status check.The
isFullyMixedmethod only reads transaction data to determine mixing status. The read lock usage enables concurrent queries while maintaining thread safety.Also applies to: 460-460
489-489: LGTM - Read lock appropriate for coin selection.The
selectCoinsGroupedByAddressesmethod performs read-only analysis of available coins and grouping. Using the read lock allows concurrent coin selection operations.Also applies to: 603-603
693-693: LGTM - Read lock correct for destination usage check.The
isUsedDestinationmethod only reads destination data without modification. The read lock usage is appropriate for this read-only query.Also applies to: 697-697
731-731: LGTM - Read lock suitable for available coins enumeration.The
availableCoinsmethod performs read-only traversal of unspent transactions to build a list of available coins. Using the read lock allows concurrent access to this information.Also applies to: 824-824
888-888: LGTM - Read lock appropriate for denominated amount selection.The
selectDenominatedAmountsmethod only reads coin data to select denominated amounts. The read lock usage enables concurrent selection operations while maintaining thread safety.Also applies to: 911-911
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
136-136: Good use of read-write lock for improved concurrencyReplacing
ReentrantLockwithReentrantReadWriteLockis an excellent optimization that allows multiple threads to read wallet state concurrently while maintaining exclusive access for writes.
4409-4409: Good deadlock prevention practiceExcellent defensive programming - ensuring the write lock is not held when calling the external broadcaster prevents potential deadlocks with external components.
| */ | ||
| private Set<Transaction> findDoubleSpendsAgainst(Transaction tx, Map<Sha256Hash, Transaction> candidates) { | ||
| checkState(lock.isHeldByCurrentThread()); | ||
| // checkState(lock.writeLock().isHeldByCurrentThread()); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the commented lock assertion
The checkState for write lock verification is commented out. This method findDoubleSpendsAgainst appears to be read-only (just finding transactions), so it might be called from contexts holding either read or write locks. However, this should be explicitly documented or the assertion should be updated to check for either lock type being held.
- // checkState(lock.writeLock().isHeldByCurrentThread());
+ // This method can be called with either read or write lock held
+ checkState(lock.readLock().isHeldByCurrentThread() || lock.writeLock().isHeldByCurrentThread());🏁 Script executed:
#!/bin/bash
# Display the findDoubleSpendsAgainst method to verify its lock requirements
rg -n -C5 "findDoubleSpendsAgainst" core/src/main/java/org/bitcoinj/wallet/Wallet.javaLength of output: 4099
Enforce read or write lock in findDoubleSpendsAgainst
The helper findDoubleSpendsAgainst is invoked under a read lock (and occasionally under a write lock). Restore the assertion to require that either lock is held, rather than only the write lock.
• File: core/src/main/java/org/bitcoinj/wallet/Wallet.java
• Line: 2234
- // checkState(lock.writeLock().isHeldByCurrentThread());
+ // Ensure we hold the wallet lock (read or write) when scanning for double spends.
+ checkState(
+ lock.getReadHoldCount() > 0 ||
+ lock.writeLock().isHeldByCurrentThread(),
+ "Must hold read or write lock when calling findDoubleSpendsAgainst"
+ );📝 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.
| // checkState(lock.writeLock().isHeldByCurrentThread()); | |
| // Ensure we hold the wallet lock (read or write) when scanning for double spends. | |
| checkState( | |
| lock.getReadHoldCount() > 0 || | |
| lock.writeLock().isHeldByCurrentThread(), | |
| "Must hold read or write lock when calling findDoubleSpendsAgainst" | |
| ); |
🤖 Prompt for AI Agents
In core/src/main/java/org/bitcoinj/wallet/Wallet.java at line 2234, the
commented out assertion currently checks only for the write lock being held, but
the method findDoubleSpendsAgainst can be called with either a read or write
lock. Modify the assertion to verify that the current thread holds either the
read lock or the write lock before proceeding, restoring the concurrency safety
check accordingly.
Issue being fixed or feature implemented
What was done?
Replace ReentrantLock with ReentrantReadWriteLock in Wallet for the main lock.
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit