fix: change Wallet.lock to ReentrantReadWriteLock#283
fix: change Wallet.lock to ReentrantReadWriteLock#283HashEngineering merged 18 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces exclusive locks with read-write locks across Bitcoin wallet components to improve concurrent access, adds parallelized wallet serialization for large wallets with profiling instrumentation, introduces graceful executor shutdown handling, and expands key chain management with new public methods for querying total issued keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Thread
participant Executor as Worker Pool
participant TxTask as Transaction Task
participant KeyTask as Keys Task
participant ExtTask as Extensions Task
participant FriendTask as Friends Task
Main->>Main: Check complexityScore
alt complexityScore > threshold
Main->>Main: Capture wallet data (snapshot)
Main->>Executor: Submit TxTask
Main->>Executor: Submit KeyTask
Main->>Executor: Submit ExtTask
Main->>Executor: Submit FriendTask
Main->>Main: Wait for all futures
par Parallel Execution
Executor->>TxTask: serialize(transactions)
Executor->>KeyTask: serialize(keys)
Executor->>ExtTask: serialize(extensions)
Executor->>FriendTask: serialize(friends)
and
TxTask-->>Executor: txData
KeyTask-->>Executor: keyData
ExtTask-->>Executor: extData
FriendTask-->>Executor: friendData
end
Executor-->>Main: allOf complete
Main->>Main: Assemble final proto
Main->>Main: Set isLastSaveParallel=true
else complexityScore <= threshold
Main->>Main: Serialize serially
Main->>Main: Set isLastSaveParallel=false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 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.
HashEngineering
force-pushed
the
fix/wallet-read-write-lock
branch
from
c248cae to
be51a29
Compare
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 (5)
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (4)
332-424:⚠️ Potential issue | 🔴 CriticalData race: Writing to
mapOutpointRoundsCachewhile holding a read lock.This method writes to
mapOutpointRoundsCache(a regularHashMap) at multiple locations (lines 346, 357, 367, 376, 385, 395, 418) while holding only a read lock. WithReentrantReadWriteLock, multiple threads can acquire the read lock simultaneously, leading to concurrent writes to theHashMapwhich is not thread-safe.This will cause data races and potential corruption of the cache.
Options to fix:
- Use
lock.writeLock()instead oflock.readLock()for this method- Change
mapOutpointRoundsCachetoConcurrentHashMapwith atomic operations- Use a separate lock specifically for the cache
🔒 Recommended fix: Use write lock for this method
int getRealOutpointCoinJoinRounds(TransactionOutPoint outpoint, int rounds) { - lock.readLock().lock(); + lock.writeLock().lock(); try { // ... method body unchanged ... } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } }
433-462:⚠️ Potential issue | 🟠 MajorInherits data race from
getRealOutpointCoinJoinRounds.While this method itself doesn't mutate state, it calls
getRealOutpointCoinJoinRounds()(line 436) which writes to the cache. If the calling method is fixed to use a write lock, this method should also use a write lock, or the cache-writing method needs a different synchronization strategy.
479-605:⚠️ Potential issue | 🔴 CriticalData race: Writing to cache fields while holding a read lock.
This method writes to multiple cache fields while holding only a read lock:
- Line 586:
vecAnonymizableTallyCachedNonDenom = vecTallyRet- Line 587:
anonymizableTallyCachedNonDenom = true- Line 589:
vecAnonymizableTallyCached = vecTallyRet- Line 590:
anonymizableTallyCached = trueMultiple threads holding read locks can concurrently execute these writes, causing race conditions on both the boolean flags and the ArrayList references.
🔒 Recommended fix: Use write lock for this method
public List<CompactTallyItem> selectCoinsGroupedByAddresses(boolean skipDenominated, boolean anonymizable, boolean skipUnconfirmed, int maxOutpointsPerAddress) { List<TransactionOutput> candidates = calculateAllSpendCandidates(true, true); CoinSelection selection = skipUnconfirmed ? DefaultCoinSelector.get().select(MAX_MONEY, candidates) : ZeroConfCoinSelector.get().select(MAX_MONEY, candidates); - lock.readLock().lock(); + lock.writeLock().lock(); try { // ... method body unchanged ... } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } }
724-826:⚠️ Potential issue | 🟠 MajorInherits data race through method calls.
This method calls
isFullyMixed()(lines 766, 770, 794) which transitively callsgetRealOutpointCoinJoinRounds()that writes to the cache. Since the calling code already holds a read lock here, and the nested call ingetRealOutpointCoinJoinRoundsattempts to acquire another read lock (which succeeds due to reentrancy), the cache mutation still occurs without exclusive access.If
getRealOutpointCoinJoinRoundsis changed to use a write lock, this method would attempt a lock upgrade (read → write) which causes deadlock withReentrantReadWriteLock. The locking strategy needs to be consistent across the call hierarchy.core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
4135-4155:⚠️ Potential issue | 🟠 MajorUse write lock in
getBalanceFutureto protect list mutation.The
balanceFutureRequestslist is marked@GuardedBy("lock")and is modified with.add(req)under a read lock. Read locks allow concurrent readers, creating a data race when multiple threads call this method simultaneously. Additionally,checkBalanceFuturesLocked()explicitly verifies the write lock is held before accessing this list, confirming write-lock protection is required.🔧 Proposed fix
- lock.readLock().lock(); + lock.writeLock().lock(); try { final SettableFuture<Coin> future = SettableFuture.create(); final Coin current = getBalance(type); if (current.compareTo(value) >= 0) { // Already have enough. future.set(current); } else { // Will be checked later in checkBalanceFutures. We don't just add an event listener for ourselves // here so that running getBalanceFuture().get() in the user code thread works - generally we must // avoid giving the user back futures that require the user code thread to be free. BalanceFutureRequest req = new BalanceFutureRequest(); req.future = future; req.value = value; req.type = type; balanceFutureRequests.add(req); } return future; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); }
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java`:
- Line 4410: The current guard only checks write-lock ownership
(checkState(!lock.writeLock().isHeldByCurrentThread())) which allows callers
holding a read lock to call sendCoins -> sendCoinsOffline and deadlock on a
write-lock upgrade; update the guard in Wallet.java to also assert the current
thread holds no read locks (e.g. checkState(lock.getReadHoldCount() == 0 &&
!lock.writeLock().isHeldByCurrentThread())) so sendCoins/sendCoinsOffline cannot
be invoked while the thread already has a read lock.
be51a29 to
bd029a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
4278-4299:⚠️ Potential issue | 🔴 CriticalUse write lock when enqueueing balance futures.
getBalanceFuturemutatesbalanceFutureRequestsunder a read lock (line 4294), which allows concurrent modifications. SincebalanceFutureRequestsis marked@GuardedBy("lock")andcheckBalanceFuturesLockedcorrectly acquires the write lock before accessing it, the read lock here is insufficient for thread safety. Switch to the write lock for this method.🔧 Suggested fix
- lock.readLock().lock(); + lock.writeLock().lock(); try { final SettableFuture<Coin> future = SettableFuture.create(); final Coin current = getBalance(type); if (current.compareTo(value) >= 0) { @@ } return future; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); }
1144-1159:⚠️ Potential issue | 🟠 MajorAcquire
keyChainGroupLockwhile mutatingwatchedScripts.The
watchedScriptsfield is annotated with@GuardedBy("keyChainGroupLock")(line 194), and all reader methods correctly acquirekeyChainGroupLockbefore accessing it. However,removeWatchedScriptsmutateswatchedScriptsunder the wallet lock only, creating a race condition with readers that holdkeyChainGroupLock. Follow the pattern used inaddWatchedScripts(which also needs fixing, but separately): acquirekeyChainGroupLockduring the mutation, respecting the documented lock ordering (wallet lock >keyChainGroupLock).🔧 Suggested fix
- lock.writeLock().lock(); - try { - for (final Script script : scripts) { - if (!watchedScripts.contains(script)) - continue; - - watchedScripts.remove(script); - } - - queueOnScriptsChanged(scripts, false); - saveNow(); - return true; - } finally { - lock.writeLock().unlock(); - } + lock.writeLock().lock(); + try { + keyChainGroupLock.lock(); + try { + for (final Script script : scripts) { + if (!watchedScripts.contains(script)) + continue; + + watchedScripts.remove(script); + } + } finally { + keyChainGroupLock.unlock(); + } + + queueOnScriptsChanged(scripts, false); + saveNow(); + return true; + } finally { + lock.writeLock().unlock(); + }
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java`:
- Line 110: The field lastSaveParallel is not thread-safe; to fix, change its
declaration to a thread-safe variant (e.g., make lastSaveParallel volatile or
replace it with an AtomicBoolean) and update all accesses in methods such as
walletToProto(...) and isLastSaveParallel() to read/write the chosen thread-safe
variable (use get()/set() for AtomicBoolean or direct reads/writes for volatile)
to ensure visibility across threads; ensure any compound operations are atomic
or synchronized if needed.
- Around line 281-286: serializeKeyChainGroupToProtobuf() is being called inside
the async supplier (keysFuture) while other pieces are serialized on the main
thread, risking lock contention; call wallet.serializeKeyChainGroupToProtobuf()
on the main thread before creating the CompletableFuture, measure/populate
keysTime[0] there, store the resulting List<Protos.Key> (e.g. keysLocal) and
then create keysFuture from that captured value (e.g.
CompletableFuture.completedFuture(keysLocal) or a trivial supplier), so the
actual locking happens synchronously like the other serializations and
keysFuture only wraps the already-captured data.
🧹 Nitpick comments (6)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
513-530: Consider adding test isolation forsmallWallet.The test creates
smallWalletusingwallet.getParams()which relies on the sharedwalletfield that was reset on line 519. While this should work correctly, consider whether explicitly usingPARAMS(the field defined at line 48) would be clearer for test isolation:- Wallet smallWallet = Wallet.createDeterministic(wallet.getParams(), Script.ScriptType.P2PKH); + Wallet smallWallet = Wallet.createDeterministic(PARAMS, Script.ScriptType.P2PKH);This is a minor readability improvement to make the test's intent clearer.
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (3)
266-268: Consider using a cached or shared thread pool instead of creating a new one per serialization.Creating a new
ExecutorServicewith 4 threads for eachwalletToProtocall on large wallets introduces overhead. If wallets are serialized frequently, this could lead to thread churn.Consider using a shared, lazily-initialized thread pool or a
ForkJoinPool.commonPool()variant.
382-390: Clarify the intent of completion-order joining for extensions and transactions.The
anyOfpattern here doesn't provide a performance benefit since both futures must complete before the builder can be finalized. The order ofaddAllExtensionvsaddAllTransactionin the protobuf is unlikely to affect performance.This pattern adds complexity without clear benefit. A simpler approach would be to join both futures directly.
♻️ Simplified approach
- // Join extensions and transactions in completion order - whichever finishes first gets added first - CompletableFuture.anyOf(extensionsFuture, txFuture).join(); - if (extensionsFuture.isDone()) { - walletBuilder.addAllExtension(extensionsFuture.join()); - walletBuilder.addAllTransaction(txFuture.join()); - } else { - walletBuilder.addAllTransaction(txFuture.join()); - walletBuilder.addAllExtension(extensionsFuture.join()); - } + walletBuilder.addAllExtension(extensionsFuture.join()); + walletBuilder.addAllTransaction(txFuture.join());
404-406: Executor shutdown without awaiting termination may leave tasks incomplete on exceptions.If an exception occurs after
executor.shutdown()in the finally block, any running tasks will be interrupted but not awaited. Consider addingawaitTerminationwith a timeout for cleaner shutdown.♻️ Proposed fix
} finally { executor.shutdown(); + try { + if (!executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS)) { + executor.shutdownNow(); + } + } catch (InterruptedException e) { + executor.shutdownNow(); + Thread.currentThread().interrupt(); + } }core/src/main/java/org/bitcoinj/wallet/KeyChainGroupExtension.java (1)
99-99: Consider adding Javadoc for the newgetTotalIssuedKeys()method.The interface method lacks documentation. Adding a brief Javadoc comment would clarify the method's purpose and return value semantics (e.g., does it include lookahead keys? basic keys?).
📝 Proposed documentation
int getCombinedKeyLookaheadEpochs(); + /** + * Returns the total number of keys issued across all key chains, + * including basic keys and leaf keys from deterministic chains. + * Unlike {`@link` `#numKeys`()}, this excludes lookahead keys. + */ int getTotalIssuedKeys();core/src/main/java/org/bitcoinj/wallet/AbstractKeyChainGroupExtension.java (1)
547-551: Consider acquiringkeyChainGroupLockfor thread safety.Most methods in this class acquire
keyChainGroupLockbefore accessinggetKeyChainGroup(), butgetTotalIssuedKeys()does not. While this is consistent withgetCombinedKeyLookaheadEpochs()andhasKeyChains(), it may lead to inconsistent reads in concurrent scenarios.🔒 Proposed fix for thread safety
`@Override` public int getTotalIssuedKeys() { - return hasKeyChains() ? getKeyChainGroup().getTotalIssuedKeys() : 0; + keyChainGroupLock.lock(); + try { + return hasKeyChains() ? getKeyChainGroup().getTotalIssuedKeys() : 0; + } finally { + keyChainGroupLock.unlock(); + } }
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java
Outdated
Show resolved
Hide resolved
| CompletableFuture<List<Protos.Key>> keysFuture = CompletableFuture.supplyAsync(() -> { | ||
| long start = System.currentTimeMillis(); | ||
| List<Protos.Key> keys = wallet.serializeKeyChainGroupToProtobuf(); | ||
| keysTime[0] = System.currentTimeMillis() - start; | ||
| return keys; | ||
| }, executor); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find serializeKeyChainGroupToProtobuf implementation and its locking behavior
rg -n "serializeKeyChainGroupToProtobuf" --type=java -A 5Repository: dashpay/dashj
Length of output: 2132
🏁 Script executed:
#!/bin/bash
# Examine the context around lines 281-286 in WalletProtobufSerializer.java
# First, get the file path
find . -name "WalletProtobufSerializer.java" -type fRepository: dashpay/dashj
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Get broader context around the problematic code section (lines 270-330)
find . -name "WalletProtobufSerializer.java" -type f -exec sed -n '270,330p' {} +Repository: dashpay/dashj
Length of output: 3455
Inconsistent serialization pattern: serializeKeyChainGroupToProtobuf() acquires lock in async context while other data is captured on main thread.
serializeKeyChainGroupToProtobuf() internally acquires keyChainGroupLock (Wallet.java:1668-1672), but this call happens inside the async task at line 283. In contrast, other serialized data (walletTransactions, walletExtensions, keyChainExtensions, receivingFromFriends, sendingToFriends) are all captured on the main thread before async dispatch. This inconsistency could lead to lock contention or deadlock if the caller holds wallet-level locks while waiting for futures to complete.
Capture the serialized key chain group on the main thread before dispatching async tasks, matching the pattern used for other data.
🤖 Prompt for AI Agents
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java` around
lines 281 - 286, serializeKeyChainGroupToProtobuf() is being called inside the
async supplier (keysFuture) while other pieces are serialized on the main
thread, risking lock contention; call wallet.serializeKeyChainGroupToProtobuf()
on the main thread before creating the CompletableFuture, measure/populate
keysTime[0] there, store the resulting List<Protos.Key> (e.g. keysLocal) and
then create keysFuture from that captured value (e.g.
CompletableFuture.completedFuture(keysLocal) or a trivial supplier), so the
actual locking happens synchronously like the other serializations and
keysFuture only wraps the already-captured data.
There was a problem hiding this comment.
if we do this then we loose the advantage of the async task
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to writeWallet and walletToProto to understand the locking context
rg -n "writeWallet\s*\(" --type=java -B 5 -A 2 | head -100
Length of output: 11019
🏁 Script executed:
#!/bin/bash
# Search for calls to walletToProto
rg -n "walletToProto\s*\(" --type=java -B 5 -A 2
Length of output: 20008
🏁 Script executed:
#!/bin/bash
# Check if Wallet class has saveToFile or similar methods that might hold locks
rg -n "saveToFile|saveToFileStream" --type=java -B 3 -A 10 core/src/main/java/org/bitcoinj/wallet/Wallet.java
Length of output: 7657
🏁 Script executed:
#!/bin/bash
# Check what kind of lock is held during save operations in Wallet
rg -n "\.writeLock\(\)|\.readLock\(\)" --type=java -B 2 -A 5 core/src/main/java/org/bitcoinj/wallet/Wallet.java | grep -A 5 -B 2 "save\|write\|serialize"
Length of output: 33511
🏁 Script executed:
#!/bin/bash
# Check the implementation of serializeKeyChainGroupToProtobuf to see what locks it acquires
rg -n "serializeKeyChainGroupToProtobuf" --type=java -A 15 core/src/main/java/org/bitcoinj/wallet/Wallet.java
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Check if there are any methods that serialize without acquiring locks
rg -n "class.*KeyChainGroup" --type=java -A 3 core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java | head -20
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Look for the actual serialization in KeyChainGroup
rg -n "serializeToProtobuf|toProtobuf" --type=java -B 2 -A 10 core/src/main/java/org/bitcoinj/wallet/KeyChainGroup.java | head -50
Length of output: 1124
@HashEngineering You're right that pre-serializing on the main thread would eliminate the parallelization benefit for that component. Let me clarify the actual risk and suggest alternatives:
The Deadlock Scenario
The concern is about lock ordering. Looking at line 1668, serializeKeyChainGroupToProtobuf() acquires keyChainGroupLock. Since saveToFileStream() holds lock.writeLock() when calling walletToProto(), and your code's locking rule states lock > keyChainGroupLock (line 134-135), the async task technically violates this ordering by acquiring keyChainGroupLock while the main thread holds lock.
However, you're correct that moving it to the main thread defeats the purpose of parallelization.
Alternative Solutions (preserving async benefit)
-
Release the write lock before waiting on futures (most beneficial):
// After capturing all data on main thread (line ~264) // ... create all futures ... // Release write lock before joining } finally { lock.writeLock().unlock(); } // Join futures outside the lock walletBuilder.addAllKey(keysFuture.join());
This eliminates the deadlock risk entirely since no lock is held while waiting.
-
Pre-capture keychain data without locks:
Create a version of serialize that doesn't acquire locks internally, or capture the actual keychain objects on the main thread:KeyChainGroup keyChainGroupSnapshot = wallet.keyChainGroup; // capture reference // Then in async task, call non-locking serialize on the snapshot
-
Accept the current risk as minimal: If
keyChainGroupLockis never held while trying to acquire the mainlock, the actual deadlock risk is very low. The inconsistency is mainly a code hygiene issue.
Which approach would you prefer? Option 1 provides the best balance of safety and performance.
There was a problem hiding this comment.
3 for now, will consider this more later.
We could reduce blocking other wallet processes after gathering wallet data, and then do the other serialization and IO in parallel.
There was a problem hiding this comment.
@HashEngineering Sounds good—option 3 is reasonable given the low actual risk in practice.
Your future optimization idea makes sense: you could gather all wallet data under the write lock (ensuring consistency), release the lock, then perform the CPU-intensive serialization and I/O completely in parallel. This would minimize contention for other wallet operations while still gaining the parallelization benefits. Feel free to tag me when you're ready to revisit this, and I'd be happy to review the implementation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
1144-1158:⚠️ Potential issue | 🟠 MajorGuard
watchedScriptswithkeyChainGroupLockto avoid races.Line 1144 currently takes the wallet write lock, but
watchedScriptsis guarded bykeyChainGroupLockand many readers only take that lock. This leaves a data race betweenremoveWatchedScriptsand readers likeisWatchedScript/getWatchedAddresses. Please lockkeyChainGroupLock(or both locks in the documented order) when mutatingwatchedScripts.🔧 Suggested fix
- lock.writeLock().lock(); + keyChainGroupLock.lock(); try { for (final Script script : scripts) { if (!watchedScripts.contains(script)) continue; @@ - } finally { - lock.writeLock().unlock(); - } + } finally { + keyChainGroupLock.unlock(); + }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
1144-1159:⚠️ Potential issue | 🟠 MajorAcquire
keyChainGroupLockwhen accessingwatchedScripts.The
watchedScriptsfield is annotated@GuardedBy("keyChainGroupLock"), but this method only holds the wallet lock. Other methods likegetWatchedAddresses()properly acquirekeyChainGroupLockbefore accessingwatchedScripts, creating a race condition. AcquirekeyChainGroupLockafter the wallet lock (respecting the documented ordering constraint).🔧 Proposed fix
public boolean removeWatchedScripts(final List<Script> scripts) { lock.writeLock().lock(); + keyChainGroupLock.lock(); try { for (final Script script : scripts) { if (!watchedScripts.contains(script)) continue; watchedScripts.remove(script); } queueOnScriptsChanged(scripts, false); saveNow(); return true; } finally { + keyChainGroupLock.unlock(); lock.writeLock().unlock(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java` around lines 1144 - 1159, removeWatchedScripts acquires only the wallet `lock` but reads/writes `watchedScripts` which is `@GuardedBy`("keyChainGroupLock"); to fix, after acquiring the existing wallet `lock` (the `lock` writeLock), also acquire `keyChainGroupLock`'s write lock (respecting the documented lock ordering: wallet lock then keyChainGroupLock), perform the contains/remove loop while holding both locks, then release keyChainGroupLock before unlocking the wallet lock; ensure `queueOnScriptsChanged(...)` and `saveNow()` are still called under the correct lock context per existing semantics.
4278-4299:⚠️ Potential issue | 🔴 CriticalSwitch to write lock to safely mutate
balanceFutureRequests.The method holds only a read lock but mutates a shared list guarded by
lock. Multiple concurrent readers can executebalanceFutureRequests.add(req)simultaneously, breaking thread-safety guarantees. The@GuardedBy("lock")annotation onbalanceFutureRequestsand the write-lock assertion incheckBalanceFuturesLocked()both indicate mutations require the write lock.🔧 Proposed fix
public ListenableFuture<Coin> getBalanceFuture(final Coin value, final BalanceType type) { - lock.readLock().lock(); + lock.writeLock().lock(); try { final SettableFuture<Coin> future = SettableFuture.create(); final Coin current = getBalance(type); if (current.compareTo(value) >= 0) { // Already have enough. future.set(current); } else { // Will be checked later in checkBalanceFutures. We don't just add an event listener for ourselves // here so that running getBalanceFuture().get() in the user code thread works - generally we must // avoid giving the user back futures that require the user code thread to be free. BalanceFutureRequest req = new BalanceFutureRequest(); req.future = future; req.value = value; req.type = type; balanceFutureRequests.add(req); } return future; } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java` around lines 4278 - 4299, getBalanceFuture currently acquires only the read lock but may mutate the guarded list balanceFutureRequests; change the locking to acquire the write lock instead (replace lock.readLock().lock()/unlock() with lock.writeLock().lock()/unlock()) so any branch that adds a BalanceFutureRequest is protected, keeping the existing try/finally structure and leaving the rest of the method (creating SettableFuture, calling getBalance(type), comparing values, adding req to balanceFutureRequests) intact; this aligns getBalanceFuture with checkBalanceFuturesLocked()'s write-lock expectation.core/src/main/java/org/bitcoinj/wallet/WalletEx.java (3)
504-618:⚠️ Potential issue | 🔴 CriticalCache mutation under read lock in
selectCoinsGroupedByAddresses.Lines 599–606 write to
vecAnonymizableTallyCachedNonDenom,anonymizableTallyCachedNonDenom,vecAnonymizableTallyCached, andanonymizableTallyCachedwhile holding onlylock.readLock(). Concurrent readers can race on these writes, producing torn reads/writes of the boolean + list pair.This method should either hold the write lock (since it mutates shared state) or the caching fields should use
volatile/atomic references.Simplest fix: use write lock
- lock.readLock().lock(); + lock.writeLock().lock(); try { // ... existing body ... } finally { - lock.readLock().unlock(); + lock.writeLock().unlock(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java` around lines 504 - 618, selectCoinsGroupedByAddresses currently mutates shared cache fields (vecAnonymizableTallyCachedNonDenom, anonymizableTallyCachedNonDenom, vecAnonymizableTallyCached, anonymizableTallyCached) while holding only lock.readLock(), causing races; fix by performing the cache writes under lock.writeLock() instead: release the read lock, acquire lock.writeLock(), (re-check the same maxOutpointsPerAddress/anonymizable/skipUnconfirmed/skipDenominated conditions if needed to avoid double-write), assign the cache fields, then release the write lock (optionally reacquire the read lock if subsequent code needs it). Ensure this change is applied inside selectCoinsGroupedByAddresses and only around the cache assignment block.
204-241:⚠️ Potential issue | 🔴 CriticalRead lock usage in
getBalancelooks correct.This method only reads wallet state (
calculateAllSpendCandidates, iterating outputs) and doesn't mutate shared fields. The read lock is appropriate here.However, note that
isFullyMixed(out)at line 220 internally callsgetRealOutpointCoinJoinRoundswhich writes tomapOutpointRoundsCache— so this path inherits the write-under-read-lock issue flagged above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java` around lines 204 - 241, getBalance holds the read lock but calls isFullyMixed(out), which via getRealOutpointCoinJoinRounds mutates mapOutpointRoundsCache—this causes a write-under-read-lock violation; fix by eliminating mutations while under the read lock: either make getRealOutpointCoinJoinRounds/read-paths side-effect free (do not update mapOutpointRoundsCache) and expose a read-only accessor used by isFullyMixed, or change getBalance to perform the coinjoin fullness check under the write lock (release readLock(), acquire writeLock() around isFullyMixed calls) so mutations happen only while holding the write lock; update references to isFullyMixed, getRealOutpointCoinJoinRounds and mapOutpointRoundsCache accordingly.
343-352:⚠️ Potential issue | 🔴 CriticalFix lock acquisition in
markAsFullyMixed()—uselock.writeLock()instead oflock.
ReentrantReadWriteLockdoes not havelock()orunlock()methods. The inheritedlockfield requires callingwriteLock().lock()andwriteLock().unlock(). Replace:lock.lock(); // ... lock.unlock();with:
lock.writeLock().lock(); // ... lock.writeLock().unlock();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java` around lines 343 - 352, The method markAsFullyMixed currently uses lock.lock()/lock.unlock() but the class uses a ReentrantReadWriteLock; update markAsFullyMixed to acquire and release the write lock via lock.writeLock().lock() before mutating mapOutpointRoundsCache and lock.writeLock().unlock() in the finally block so the mapOutpointRoundsCache put(outPoint, 19) is protected by the write lock.
🧹 Nitpick comments (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
521-538: Test looks reasonable and covers the parallel/serial serialization threshold.The test validates that large wallets (even after reset, presumably due to key chain size) use parallel serialization, while small fresh wallets use serial. Good coverage of the
isLastSaveParallel()API.One observation: line 529 asserts
isLastSaveParallel()istrueafterwallet.reset(). This implicitly tests that the parallel threshold is based on something other than transaction count alone (since the reset wallet has 0 transactions per line 532). If the threshold logic changes, this assertion may break unexpectedly. Consider adding a brief comment explaining why a reset wallet still triggers parallel mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java` around lines 521 - 538, Add a brief inline comment above the assertTrue(serializer.isLastSaveParallel()) after wallet.reset() explaining that WalletProtobufSerializer's parallel/serial decision is based on the wallet's keychain/HD seed size or other metadata (not just transaction count), so a reset wallet with cleared transactions can still exceed the parallel-threshold; reference WalletProtobufSerializer and wallet.reset() so future readers understand why the test expects parallel serialization despite zero transactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java`:
- Around line 4488-4496: sendCoinsOffline can deadlock if the calling thread
holds the read lock because it unconditionally acquires the write lock; add the
same read-lock-upgrade guard used in sendCoins to bail out with an exception (or
temporarily release/upgrade) when the current thread holds the read lock before
acquiring the write lock. Specifically, in Wallet.sendCoinsOffline check the
ReentrantReadWriteLock's read lock hold count for the current thread (same logic
used in sendCoins) and throw the same IllegalStateException (or follow the
existing pattern) instead of attempting to acquire the write lock while a read
lock is held; this prevents self-deadlock while preserving the existing
completeTx(request) and commitTx(request.tx) behavior.
In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java`:
- Around line 303-310: The fix: replace invalid lock.lock()/unlock() uses in
markAsFullyMixed with lock.writeLock().lock()/unlock() since markAsFullyMixed
mutates state, and address the cache-concurrency bug by either (A) acquiring the
write lock in methods that mutate mapOutpointRoundsCache (e.g.,
getRealOutpointCoinJoinRounds should not perform HashMap.put() while holding
lock.readLock(), and selectCoinsGroupedByAddresses must not mutate caches under
readLock), or (B) convert mapOutpointRoundsCache to a thread-safe
ConcurrentHashMap and leave read locks unchanged; choose one approach and apply
consistently so that any code paths that call mapOutpointRoundsCache.put(...)
run under lock.writeLock() or into a ConcurrentHashMap, updating usages in
getRealOutpointCoinJoinRounds, markAsFullyMixed, and
selectCoinsGroupedByAddresses accordingly.
---
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java`:
- Around line 1144-1159: removeWatchedScripts acquires only the wallet `lock`
but reads/writes `watchedScripts` which is `@GuardedBy`("keyChainGroupLock"); to
fix, after acquiring the existing wallet `lock` (the `lock` writeLock), also
acquire `keyChainGroupLock`'s write lock (respecting the documented lock
ordering: wallet lock then keyChainGroupLock), perform the contains/remove loop
while holding both locks, then release keyChainGroupLock before unlocking the
wallet lock; ensure `queueOnScriptsChanged(...)` and `saveNow()` are still
called under the correct lock context per existing semantics.
- Around line 4278-4299: getBalanceFuture currently acquires only the read lock
but may mutate the guarded list balanceFutureRequests; change the locking to
acquire the write lock instead (replace lock.readLock().lock()/unlock() with
lock.writeLock().lock()/unlock()) so any branch that adds a BalanceFutureRequest
is protected, keeping the existing try/finally structure and leaving the rest of
the method (creating SettableFuture, calling getBalance(type), comparing values,
adding req to balanceFutureRequests) intact; this aligns getBalanceFuture with
checkBalanceFuturesLocked()'s write-lock expectation.
In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java`:
- Around line 504-618: selectCoinsGroupedByAddresses currently mutates shared
cache fields (vecAnonymizableTallyCachedNonDenom,
anonymizableTallyCachedNonDenom, vecAnonymizableTallyCached,
anonymizableTallyCached) while holding only lock.readLock(), causing races; fix
by performing the cache writes under lock.writeLock() instead: release the read
lock, acquire lock.writeLock(), (re-check the same
maxOutpointsPerAddress/anonymizable/skipUnconfirmed/skipDenominated conditions
if needed to avoid double-write), assign the cache fields, then release the
write lock (optionally reacquire the read lock if subsequent code needs it).
Ensure this change is applied inside selectCoinsGroupedByAddresses and only
around the cache assignment block.
- Around line 204-241: getBalance holds the read lock but calls
isFullyMixed(out), which via getRealOutpointCoinJoinRounds mutates
mapOutpointRoundsCache—this causes a write-under-read-lock violation; fix by
eliminating mutations while under the read lock: either make
getRealOutpointCoinJoinRounds/read-paths side-effect free (do not update
mapOutpointRoundsCache) and expose a read-only accessor used by isFullyMixed, or
change getBalance to perform the coinjoin fullness check under the write lock
(release readLock(), acquire writeLock() around isFullyMixed calls) so mutations
happen only while holding the write lock; update references to isFullyMixed,
getRealOutpointCoinJoinRounds and mapOutpointRoundsCache accordingly.
- Around line 343-352: The method markAsFullyMixed currently uses
lock.lock()/lock.unlock() but the class uses a ReentrantReadWriteLock; update
markAsFullyMixed to acquire and release the write lock via
lock.writeLock().lock() before mutating mapOutpointRoundsCache and
lock.writeLock().unlock() in the finally block so the mapOutpointRoundsCache
put(outPoint, 19) is protected by the write lock.
---
Duplicate comments:
In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java`:
- Around line 751-845: The method currently acquires lock.readLock() but calls
isFullyMixed(...) which calls getRealOutpointCoinJoinRounds(...) and mutates a
cache; change the locking so writes are protected: replace the read lock with
lock.writeLock() for the region that may call
isFullyMixed/getRealOutpointCoinJoinRounds (or acquire the write lock around
each isFullyMixed(...) invocation), ensuring lock.writeLock().lock() is used
before calling isFullyMixed(...) and unlocked in the finally block; reference
the existing lock variable and the isFullyMixed/getRealOutpointCoinJoinRounds
symbols when making the change.
---
Nitpick comments:
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java`:
- Around line 521-538: Add a brief inline comment above the
assertTrue(serializer.isLastSaveParallel()) after wallet.reset() explaining that
WalletProtobufSerializer's parallel/serial decision is based on the wallet's
keychain/HD seed size or other metadata (not just transaction count), so a reset
wallet with cleared transactions can still exceed the parallel-threshold;
reference WalletProtobufSerializer and wallet.reset() so future readers
understand why the test expects parallel serialization despite zero
transactions.
2 participants
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
Release Notes
New Features
Bug Fixes
Optimizations