fix: change Wallet.lock to ReentrantReadWriteLock#283
fix: change Wallet.lock to ReentrantReadWriteLock#283HashEngineering wants to merge 15 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdded a ReentrantReadWriteLock factory and migrated Wallet synchronization from a single ReentrantLock to ReentrantReadWriteLock; introduced complexity-driven, parallelized wallet protobuf serialization with profiling; added getTotalIssuedKeys APIs; and applied several concurrency, logging, and sizing hardenings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller/MainThread
participant Serializer as WalletProtobufSerializer
participant WorkerPool as ExecutorService
participant Futures as CompletableFutures
participant Builder as Protos.Wallet.Builder
Caller->>Serializer: walletToProto(wallet)
Serializer->>Serializer: compute complexity score
alt small complexity
Serializer->>Builder: build serially (txs, keys, extensions, friends)
Builder-->>Serializer: Protos.Wallet
Serializer-->>Caller: return proto (serial)
else large complexity
Serializer->>Serializer: capture main-thread snapshots
Serializer->>WorkerPool: submit txs (future A)
Serializer->>WorkerPool: submit keys (future B)
Serializer->>WorkerPool: submit extensions (future C)
WorkerPool-->>Futures: complete A/B/C
Futures->>Serializer: join results
Serializer->>Builder: assemble parts into Protos.Wallet
Builder-->>Serializer: Protos.Wallet
Serializer-->>Caller: return proto (parallel)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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.40.5)core/src/main/java/org/bitcoinj/wallet/Wallet.javaThanks 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(); + }
1 participant
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
Refactor
New Features
Bug Fixes / Robustness
Tests