fix: resolve sync issues #287
Conversation
WalkthroughAdded timeout error types and listener plumbing, executor-introspection and forceStop lifecycle control with max-stalls config, adaptive I/O buffering for DB/wallet serialization, wallet spent-outpoint indexing and save controls, plus assorted robustness tweaks and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Peer as Peer (socket)
participant Handler as PeerSocketHandler
participant PG as PeerGroup
participant Exec as Listener Executor
participant L as TimeoutErrorListener
Peer->>Handler: socket timeout event
Handler->>Handler: capture thread & inspect stacks
Handler->>Handler: checkForBlockStoreTimeout()
alt blockstore-related detected
Handler->>PG: queueTimeoutErrorListeners(BLOCKSTORE_MEMORY_ACCESS, peerAddress)
else
Handler->>PG: queueTimeoutErrorListeners(UNKNOWN, peerAddress)
end
PG->>PG: iterate listener registrations
PG->>Exec: submit onTimeout(listener, error, peerAddress)
Exec->>L: onTimeout(error, peerAddress)
Handler->>Peer: close connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java (2)
44-47: Critical: Missing exit after error message causes runtime exception.When
args.length < 1, the error message is printed but the program continues executing, leading to anArrayIndexOutOfBoundsExceptionat line 53 when attempting to accessargs[0].Apply this diff to exit after printing the error:
if (args.length < 1) { System.out.println("RestoreFromSeedThenDump network \"seed phrase\" [--debuglog]"); System.out.println(" missing the network"); + return; }
65-68: Critical: Missing exit after error message causes runtime exception.When
args.length <= 1, the error message is printed but the program continues executing, leading to anArrayIndexOutOfBoundsExceptionat line 69 when attempting to accessargs[1].Apply this diff to exit after printing the error:
if (args.length <= 1) { System.out.println("RestoreFromSeedThenDump network \"seed phrase\" [--debuglog]"); System.out.println(" missing the seed phrase"); + return; }core/src/main/java/org/bitcoinj/core/PeerGroup.java (1)
2234-2264: Critical: maxStalls is never reset and has potential race conditions.The stall logic has several issues:
No reset mechanism: Once
maxStallsreaches 0 after repeated stalls (line 2246), stall disconnections are permanently disabled for the lifetime of thisPeerGroupinstance. This means if network conditions improve later, the system won't recover the ability to disconnect stalled peers.TOCTOU race condition: Line 2234 reads
currentMaxStallsviagetMaxStalls(), but between this read and the synchronized block at lines 2244-2250, another thread could callsetMaxStalls(), causing inconsistent behavior.Decrement semantics: The decrement at line 2246 permanently modifies the field, which may not be the intended design. Consider whether this should be a counter that resets, or a persistent configuration.
Consider one of these approaches:
Option 1: Use a separate stall counter that can reset:
@GuardedBy("lock") private int maxStalls = 3; +@GuardedBy("lock") private int currentStallCount = 0;Then in the calculator:
- int currentMaxStalls = getMaxStalls(); - if (currentMaxStalls <= 0) { + int configuredMaxStalls = getMaxStalls(); + if (configuredMaxStalls <= 0) { log.info(statsString + ", stall disabled " + thresholdString); } else if (warmupSeconds > 0) { // ... warmup logic } else if (average < minSpeedBytesPerSec && !waitForPreBlockDownload) { - log.info("{}, STALLED {}, maxStalls: {}", statsString, thresholdString, currentMaxStalls); + log.info("{}, STALLED {}, maxStalls: {}", statsString, thresholdString, configuredMaxStalls); lock.lock(); try { - PeerGroup.this.maxStalls--; - currentMaxStalls = PeerGroup.this.maxStalls; + currentStallCount++; } finally { lock.unlock(); } - if (currentMaxStalls == 0) { + if (currentStallCount >= configuredMaxStalls) { log.warn("This network seems to be slower than the requested stall threshold - won't do stall disconnects any more."); + // Optionally reset: currentStallCount = 0; } else { Peer peer = getDownloadPeer(); log.warn(String.format(Locale.US, "Chain download stalled: received %.2f KB/sec for %d seconds, require average of %.2f KB/sec, disconnecting %s, %d stalls left", - average / 1024.0, samples.length, minSpeedBytesPerSec / 1024.0, peer, currentMaxStalls)); + average / 1024.0, samples.length, minSpeedBytesPerSec / 1024.0, peer, configuredMaxStalls - currentStallCount));Option 2: Reset the stall count on successful sync or peer change:
Add a method to reset stalls when conditions improve (e.g., whensyncDonebecomes true at line 2216).
🧹 Nitpick comments (5)
examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java (1)
48-52: Approve conditional logging with suggestions for improvement.The conditional logging initialization is a useful debugging feature. However, consider these improvements:
- Use
args.length >= 3instead ofargs.length == 3to support potential future arguments without silently disabling verbose logging.- Document the optional flag: Update the usage messages at lines 45-46 and 66-67 to mention the optional
--debuglogparameter.Apply this diff to make the check more flexible:
- if (args.length == 3 && args[2].equals("--debuglog")) { + if (args.length >= 3 && args[2].equals("--debuglog")) {Update the usage messages to document the flag:
if (args.length < 1) { - System.out.println("RestoreFromSeedThenDump network \"seed phrase\""); + System.out.println("RestoreFromSeedThenDump network \"seed phrase\" [--debuglog]"); System.out.println(" missing the network");if (args.length <= 1) { - System.out.println("RestoreFromSeedThenDump network \"seed phrase\""); + System.out.println("RestoreFromSeedThenDump network \"seed phrase\" [--debuglog]"); System.out.println(" missing the seed phrase");core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
617-633: Move return statement outside the finally block.While the synchronization correctly protects access to
myUnspents, placing the return statement inside the finally block is unconventional and can obscure control flow. Store the result in a variable and return it after the finally block completes.Apply this diff to improve clarity:
public int countInputsWithAmount(Coin inputValue) { lock.lock(); + int count; try { - int count = 0; + count = 0; for (TransactionOutput output : myUnspents) { TransactionConfidence confidence = output.getParentTransaction().getConfidence(context); // confirmations must be 0 or higher, not conflicted or dead if (confidence != null && (confidence.getConfidenceType() == TransactionConfidence.ConfidenceType.PENDING || confidence.getConfidenceType() == TransactionConfidence.ConfidenceType.BUILDING)) { // inputValue must match, the TX is mine and is not spent if (output.getValue().equals(inputValue) && output.getSpentBy() == null) { count++; } } } - return count; } finally { lock.unlock(); } + return count; }core/src/main/java/org/bitcoinj/core/PeerGroup.java (3)
1282-1331: Refactor: Consider moving diagnostic logic to a separate method.The diagnostic logging in
stopAsync()adds significant complexity to a critical shutdown path:
- Reflection fragility: Lines 1292-1294 use reflection to access a private
delegatefield. This will break if Guava changes its internal implementation ofListeningDecorator.- Expensive thread enumeration: Lines 1307-1324 enumerate all threads in the root
ThreadGroup, which could include many unrelated threads in a large application. Consider filtering threads more precisely before enumeration.- Broad exception handling: Line 1326 catches all exceptions and only logs the message, potentially masking important diagnostic information.
Consider extracting this diagnostic logic into a separate
logExecutorState()method that can be called conditionally (e.g., when a debug flag is enabled), rather than running on every shutdown:public ListenableFuture stopAsync() { checkState(vRunning); vRunning = false; - - // Log executor state and remaining jobs - log.info("stopAsync() called - executor shutdown: {}, terminated: {}", - executor.isShutdown(), executor.isTerminated()); - - // The executor is wrapped by MoreExecutors.listeningDecorator, need to access the underlying executor - if (executor instanceof ListeningScheduledExecutorService) { - // Try to get the underlying ScheduledThreadPoolExecutor - try { - // Use reflection to access the delegate field in ListeningDecorator - java.lang.reflect.Field delegateField = executor.getClass().getDeclaredField("delegate"); - delegateField.setAccessible(true); - Object delegate = delegateField.get(executor); - - if (delegate instanceof ScheduledThreadPoolExecutor) { - ScheduledThreadPoolExecutor stpe = (ScheduledThreadPoolExecutor) delegate; - log.info("Executor queue size: {}, active threads: {}", - stpe.getQueue().size(), stpe.getActiveCount()); - - // Log remaining jobs in queue - stpe.getQueue().forEach(job -> { - log.info("Remaining job: {} (class: {})", job.toString(), job.getClass().getSimpleName()); - }); - - // Get call stacks of executor threads - ThreadGroup rootGroup = Thread.currentThread().getThreadGroup(); - while (rootGroup.getParent() != null) { - rootGroup = rootGroup.getParent(); - } - - Thread[] threads = new Thread[rootGroup.activeCount()]; - rootGroup.enumerate(threads); - - for (Thread thread : threads) { - if (thread != null && thread.getName().contains("PeerGroup Thread")) { - log.info("Found PeerGroup thread: {} (state: {})", thread.getName(), thread.getState()); - StackTraceElement[] stackTrace = thread.getStackTrace(); - log.info("Stack trace for thread {}:", thread.getName()); - for (int i = 0; i < Math.min(stackTrace.length, 15); i++) { - log.info(" at {}", stackTrace[i]); - } - } - } - } - } catch (Exception e) { - log.info("Could not access underlying executor details: {}", e.getMessage()); - } - } - + log.info("stopAsync() called"); log.info("About to submit shutdown task to executor");
1373-1401: Add documentation for thread safety and usage guidance.The
forceStop()method provides useful forced-shutdown capability, but lacks documentation about when to use it versusstop()and thread-safety considerations.Consider adding JavaDoc:
+/** + * Performs a forced shutdown of the PeerGroup after waiting for the specified timeout. + * Unlike {@link #stop()}, this method will forcibly terminate the executor if it doesn't + * shut down within the specified time. + * + * <p>This method can be called multiple times safely. If the PeerGroup is not running, + * it will still attempt to shut down the executor.</p> + * + * <p><b>Thread safety:</b> This method blocks until shutdown completes or times out.</p> + * + * @param waitMsBeforeShutdown Maximum time in milliseconds to wait for graceful shutdown + * before forcing termination + */ public void forceStop(int waitMsBeforeShutdown) {
1403-1417: Consider more robust job identification.The
getJobDescription()method uses string matching ontoString()output (lines 1408-1414), which is brittle and could break if the string representation changes.Consider using
instanceofchecks or maintaining aMap<Runnable, String>for known jobs:private String getJobDescription(Runnable job) { if (job == triggerConnectionsJob) { return "Peer connection management"; } else if (job instanceof ChainDownloadSpeedCalculator) { return "Chain download speed monitoring"; + } else if (job instanceof ScheduledFuture) { + // Unwrap ScheduledFuture to get the actual task + // Note: This requires additional logic to safely unwrap + return "Scheduled task"; } else { - return "Unknown task"; + return "Unknown task: " + job.getClass().getName(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
core/build.gradle(1 hunks)core/src/main/java/org/bitcoinj/coinjoin/CoinJoin.java(5 hunks)core/src/main/java/org/bitcoinj/core/Peer.java(6 hunks)core/src/main/java/org/bitcoinj/core/PeerGroup.java(7 hunks)core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java(1 hunks)core/src/main/java/org/bitcoinj/core/VersionMessage.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java(1 hunks)core/src/main/java/org/bitcoinj/store/FlatDB.java(5 hunks)core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(2 hunks)core/src/main/java/org/bitcoinj/wallet/WalletEx.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/WalletFiles.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java(2 hunks)core/src/test/java/org/bitcoinj/core/PeerGroupTest.java(1 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(4 hunks)examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/main/java/org/bitcoinj/coinjoin/CoinJoin.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.javacore/src/main/java/org/bitcoinj/wallet/WalletEx.javacore/src/main/java/org/bitcoinj/core/Peer.javacore/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.javacore/src/main/java/org/bitcoinj/wallet/Wallet.java
📚 Learning: 2025-09-18T22:12:09.613Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 286
File: core/src/main/java/org/bitcoinj/wallet/Wallet.java:6371-6373
Timestamp: 2025-09-18T22:12:09.613Z
Learning: In bitcoinj's Wallet.java, methods returning List<ECKey> can safely return new LinkedList<>(detkeys) where detkeys is List<DeterministicKey>, because DeterministicKey extends ECKey and Java's type inference handles this covariance correctly during list construction.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧬 Code graph analysis (4)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (3)
core/src/main/java/org/bitcoinj/core/Transaction.java (1)
Transaction(70-1867)core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java (1)
BriefLogFormatter(29-77)core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
Wallet(129-6456)
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
core/src/main/java/org/bitcoinj/core/TransactionConfidence.java (1)
TransactionConfidence(66-714)
examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java (1)
core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java (1)
BriefLogFormatter(29-77)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
core/src/main/java/org/bitcoinj/core/Sha256Hash.java (1)
Sha256Hash(38-315)
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
🔇 Additional comments (15)
core/src/main/java/org/bitcoinj/core/VersionMessage.java (1)
47-47: Version constant update is correct and consistent.The
BITCOINJ_VERSIONconstant update from"21.1.12-SNAPSHOT"to"21.1.12"is properly synchronized with the version incore/build.gradle. This public constant affects theLIBRARY_SUBVERfield used in peer identification strings (line 49), which will now report the release version to network peers.core/build.gradle (1)
13-13: Version bump verified and correctly synchronized.The version change from
21.1.12-SNAPSHOTto21.1.12is complete and consistent across the codebase. Bothcore/build.gradle(line 13) andVersionMessage.java(line 47:BITCOINJ_VERSION) reference the same version, with no orphaned SNAPSHOT versions remaining. The git diff confirms this PR includes multiple substantive changes across network communication, wallet operations, and CoinJoin functionality—aligning with the mentioned sync issue fixes.core/src/main/java/org/bitcoinj/wallet/WalletFiles.java (1)
141-141: LGTM! Enhanced observability for wallet save operations.The addition of file size and path to the log message provides valuable diagnostic information without impacting functionality.
core/src/main/java/org/bitcoinj/store/FlatDB.java (4)
29-30: LGTM! Configurable I/O buffering with sensible defaults.The default 64KB buffer size and adaptive sizing flag provide good starting points for performance optimization while maintaining flexibility.
113-135: LGTM! Well-designed adaptive buffer sizing logic.The tiered buffer sizes (4KB, 16KB, 64KB, 128KB) based on file size are appropriate for balancing memory usage and I/O performance across different file sizes.
163-169: LGTM! Proper use of buffered I/O with try-with-resources.The write path correctly uses
BufferedOutputStreamwith adaptive buffer sizing and ensures proper resource cleanup through try-with-resources.
218-223: LGTM! Proper use of buffered I/O with try-with-resources.The read path correctly uses
BufferedInputStreamwith adaptive buffer sizing and ensures proper resource cleanup through try-with-resources.core/src/main/java/org/bitcoinj/coinjoin/CoinJoin.java (4)
53-53: LGTM! Appropriate use of concurrent data structure.Replacing
HashMapwithConcurrentHashMapformapDSTXeliminates the need for explicit locking while maintaining thread-safety for DSTX management.
224-224: LGTM! Safe concurrent removal of expired entries.The
removeIfoperation onConcurrentHashMap.entrySet()is thread-safe and provides atomic removal per entry, correctly replacing the previous explicit locking approach.
250-265: LGTM! Simplified transaction flow hooks with concurrent map semantics.Removing explicit synchronization is safe here since
ConcurrentHashMapprovides thread-safe operations for the individual get/put operations withinupdateDSTXConfirmedHeight.
321-321: LGTM! Thread-safe status message map.Using
ConcurrentHashMapforstatusMessageMapis appropriate, as it's initialized in the static block and accessed concurrently for read operations.core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java (1)
60-60: LGTM! Improved log readability with thread names.Replacing numeric thread IDs with thread names makes logs more intuitive and easier to trace, especially in concurrent scenarios.
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (1)
119-135: Appreciate the richer timeout diagnostics.Dumping the responsible thread plus the key PeerGroup/NioClientManager workers will make future stalls far easier to root-cause. Nicely scoped to the timeout path.
core/src/main/java/org/bitcoinj/core/PeerGroup.java (2)
2110-2138: LGTM!The
setMaxStalls()andgetMaxStalls()methods are properly synchronized and provide a clean API for configuring stall behavior. The ability to disable stall disconnections by settingmaxStallsto 0 is a useful feature.
2087-2087: LGTM!The
maxStallsfield is properly declared with the@GuardedBy("lock")annotation and a reasonable default value of 3.
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
core/src/main/java/org/bitcoinj/core/listeners/TimeoutError.java (1)
1-6: TimeoutError enum looks good; maybe add brief docs for future clarityEnum values and naming are clear and match intended use; no functional issues spotted. Consider a short Javadoc on the enum and each constant to lock in semantics as more timeout types get added later.
core/src/main/java/org/bitcoinj/core/listeners/TimeoutErrorListener.java (1)
1-7: TimeoutErrorListener API is minimal and appropriateThe interface shape (error + PeerAddress) matches how it’s used in PeerSocketHandler/PeerGroup and is easy to implement. If you want, you could later add Javadoc describing threading context and typical callers, but it's not required for correctness.
core/src/main/java/org/bitcoinj/core/PeerGroup.java (3)
163-164: TimeoutError listener wiring is mostly solid; fix docs and consider cleanup of commented codeThe overall timeout listener design is consistent with other PeerGroup listeners and looks correct:
timeoutErrorListenersusesCopyOnWriteArrayList<ListenerRegistration<TimeoutErrorListener>>, matching patterns used elsewhere.addTimeoutErrorListenerpropagates the listener to both connected and pending peers, so late registrations see existing peers.connectTo(...)adds all existing timeout listeners to new peers before connection, so timeouts during connect can still be reported.handlePeerDeath(...)correctly removes timeout listeners from a dying peer, keeping peer‑side state clean.queueTimeoutErrorListeners(...)dispatches on the providedExecutorwithout additional locking, consistent with other queue* methods.A few small issues worth addressing:
Misleading Javadoc for
addTimeoutErrorListener
The comment referencesOnTransactionBroadcastListener, which is unrelated and will confuse users of this new API.Left‑over commented‑out code in
handleNewPeer
The timeout listener propagation there is commented://for (ListenerRegistration<TimeoutErrorListener> registration : timeoutErrorListeners) // peer.addTimeoutErrorListener(registration.executor, registration.listener);Given you already attach listeners in
connectTo(...)and inaddTimeoutErrorListener(...)for existing peers, this is redundant and can safely be removed rather than kept commented. Leaving it commented makes it unclear which path is authoritative and risks double‑registration if someone re‑enables it later.Consider:
- /** See {@link Peer#addOnTransactionBroadcastListener(OnTransactionBroadcastListener)} */ + /** Registers listeners that will be notified when any peer in this group reports a timeout. */ public void addTimeoutErrorListener(Executor executor, TimeoutErrorListener listener) { ... } @@ - //for (ListenerRegistration<TimeoutErrorListener> registration : timeoutErrorListeners) - // peer.addTimeoutErrorListener(registration.executor, registration.listener); + // TimeoutErrorListeners are already attached in connectTo(...) and via addTimeoutErrorListener(...) + // when listeners are added after peers exist.Functionally you’re in good shape; this is mainly API clarity and avoiding future confusion.
Also applies to: 915-922, 1035-1043, 1727-1728, 1905-1907, 2092-2093, 2459-2463
2112-2115: Stall configuration viamaxStallsworks but mutating the config field in-place may surprise callersThe new
maxStallsfield andsetMaxStalls/getMaxStallsmethods are correctly guarded bylockand are used safely insideChainDownloadSpeedCalculator.calculate():
- Access patterns respect the
@GuardedBy("lock")contract.- Using
getMaxStalls()insidecalculate()avoids unsynchronized reads.- Decrementing
PeerGroup.this.maxStallsunder the lock ensures consistent updates, and the single-threaded scheduled executor prevents concurrentcalculate()executions.One behavioral nuance:
maxStallsacts as both configuration (“how many stalls are allowed”) and remaining budget (gets decremented each time a stall is detected). External code callinggetMaxStalls()after a few stalls will see a reduced number, which may not be obvious from the setter’s Javadoc.If you want to keep configuration and runtime state separate (to avoid surprising API users), you could maintain:
- a constant
configuredMaxStallsset bysetMaxStalls(...), and- a private
remainingStallsmanaged only byChainDownloadSpeedCalculator.This would let
getMaxStalls()report the configured policy while still enforcing a decreasing internal limit.Also, it may be worth adding a simple guard in
setMaxStallsto reject negative values:public void setMaxStalls(int maxStalls) { checkArgument(maxStalls >= 0, "maxStalls must be >= 0"); ... }As implemented, correctness is fine; this is mainly about API semantics and future maintainability.
Also applies to: 2137-2165, 2177-2178, 2261-2278, 2291-2292
1299-1352: Reflection into Guava internals is unsupported and fragile—refactor to capture the delegate directlyThe extra logging in
stopAsync()and the newforceStop(int waitMsBeforeShutdown)method materially improve debuggability for stuck sync/shutdown scenarios:
- Logging executor shutdown/termination state and queued tasks is useful when diagnosing hangs.
forceStopgives callers a bounded wait followed byshutdownNow()and logs remaining jobs.However, the reflection approach is problematic:
The
delegatefield is an undocumented implementation detail. Guava'sMoreExecutors.listeningDecoratorreturns implementations that wrap a delegate via a private nested class, but the public javadoc and API do not promise or expose any accessibledelegatefield. Reflection on private nested class fields is unsupported and fragile across Guava versions.Recommended fix: Instead of reflecting into Guava's internals, capture and store the underlying
ScheduledThreadPoolExecutorin a private field when you callMoreExecutors.listeningDecorator()increatePrivateExecutor(). This avoids brittle reflection and gives you direct access to executor metrics (queue size, active threads, etc.) without depending on Guava's implementation details.Regarding
forceStop: consider documenting that it should not be used concurrently withstop()and that it can interrupt in-flight tasks (chain download, peer handling).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/org/bitcoinj/core/PeerGroup.java(14 hunks)core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java(5 hunks)core/src/main/java/org/bitcoinj/core/listeners/TimeoutError.java(1 hunks)core/src/main/java/org/bitcoinj/core/listeners/TimeoutErrorListener.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (2)
core/src/main/java/org/bitcoinj/net/NioClient.java (1)
NioClient(31-126)core/src/main/java/org/bitcoinj/net/NioClientManager.java (1)
NioClientManager(36-196)
core/src/main/java/org/bitcoinj/core/listeners/TimeoutErrorListener.java (1)
core/src/main/java/org/bitcoinj/core/PeerAddress.java (1)
PeerAddress(37-223)
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
🔇 Additional comments (2)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (1)
21-29: Timeout listener plumbing and data structures are appropriateUsing
CopyOnWriteArrayList<ListenerRegistration<TimeoutErrorListener>>fortimeoutErrorListenersmatches how other listener sets are handled and is thread-safe for concurrent add/remove/dispatch. Imports and field placement integrate cleanly with existing code.Also applies to: 41-43, 67-68
core/src/main/java/org/bitcoinj/core/PeerGroup.java (1)
2459-2463: Group-level timeout event dispatch mirrors other listeners and looks correct
queueTimeoutErrorListeners(TimeoutError error, PeerAddress peerAddress)follows the same pattern as other queue* methods in this class: it iterates thetimeoutErrorListenersCOW list and executes each listener on its configuredExecutor. This is thread-safe and aligns with expectations for the rest of the listener API.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (2)
146-185: Blockstore timeout detection still duplicates stack scans and only drives the boolean offpeekByteArrayThe structure here still has the two main problems previously flagged: it calls
Thread.getAllStackTraces()twice and only returnstrueforpeekByteArray, while mentioning SPVBlockStore in comments/logs and ignoringDirectByteBuffer.get. This means:
- You pay the cost of walking all stacks twice on every timeout.
- A timeout stuck in
SPVBlockStore.getorDirectByteBuffer.getwill be logged in the first pass (if you ever add those checks back) but won’t influence the boolean result that drivesTimeoutError.BLOCKSTORE_MEMORY_ACCESS.A single pass over
Thread.getAllStackTraces()that both logs and classifies, and that checks all relevant native frames (peekByteArray,SPVBlockStore.get,DirectByteBuffer.get), keeps behavior consistent and cheaper.You can refactor along these lines (imports omitted here; you’d also need
import java.util.Map;):- private boolean checkForBlockStoreTimeout() { - Thread.getAllStackTraces().forEach((thread, stackTrace) -> { - String threadName = thread.getName(); - if (threadName.contains("PeerGroup Thread") || threadName.contains("NioClientManager")) { - log.warn("Stack trace for thread '{}' (State: {}):", threadName, thread.getState()); - - boolean foundBlockingCall = false; - for (StackTraceElement element : stackTrace) { - log.warn(" at {}", element); - - // Check if this thread is stuck in native peekByteArray or SPVBlockStore operations - String elementStr = element.toString(); - if (elementStr.contains("peekByteArray")) { - foundBlockingCall = true; - } - } - - if (foundBlockingCall) { - log.error("CRITICAL: Thread '{}' is stuck in native I/O operation (peekByteArray/SPVBlockStore)", threadName); - } - } - }); - - // Check all threads for blocking SPVBlockStore calls - for (Thread thread : Thread.getAllStackTraces().keySet()) { - for (StackTraceElement element : thread.getStackTrace()) { - String elementStr = element.toString(); - if (elementStr.contains("peekByteArray")) { - log.error("CRITICAL: Detected SPVBlockStore timeout - native I/O freeze detected in thread: {}", thread.getName()); - return true; - } - } - } - return false; - } + private boolean checkForBlockStoreTimeout() { + Map<Thread, StackTraceElement[]> allStacks = Thread.getAllStackTraces(); + boolean hasBlockStoreTimeout = false; + + for (Map.Entry<Thread, StackTraceElement[]> entry : allStacks.entrySet()) { + Thread thread = entry.getKey(); + StackTraceElement[] stackTrace = entry.getValue(); + String threadName = thread.getName(); + if (threadName.contains("PeerGroup Thread") || threadName.contains("NioClientManager")) { + log.warn("Stack trace for thread '{}' (State: {}):", threadName, thread.getState()); + + boolean foundBlockingCall = false; + for (StackTraceElement element : stackTrace) { + log.warn(" at {}", element); + + String elementStr = element.toString(); + if (elementStr.contains("peekByteArray") + || elementStr.contains("SPVBlockStore.get") + || elementStr.contains("DirectByteBuffer.get")) { + foundBlockingCall = true; + } + } + + if (foundBlockingCall) { + log.error("CRITICAL: Thread '{}' is stuck in native I/O operation (peekByteArray/SPVBlockStore)", threadName); + hasBlockStoreTimeout = true; + } + } + } + + if (hasBlockStoreTimeout) { + log.error("CRITICAL: Detected SPVBlockStore timeout - native I/O freeze detected"); + } + return hasBlockStoreTimeout; + }This removes the second scan, makes the boolean result match what you log, and properly classifies all three native I/O hotspots as
BLOCKSTORE_MEMORY_ACCESS.
314-323: Fix timeout listener Javadoc and removal logging to match behaviorSame two nits as previously noted still apply here:
- Javadoc for
addTimeoutErrorListeneris copy‑pasted from the pre‑message listener API and does not describe timeout behavior.removeTimeoutErrorListenerlogstimeoutErrorListeners.size() - 1before removal and regardless of whether the listener was actually present, so the “new size” can be wrong.Consider tightening this as follows:
- /** Registers a listener that is called immediately before a message is received */ + /** Registers a listener that will be notified when this peer experiences a socket timeout. */ public void addTimeoutErrorListener(Executor executor, TimeoutErrorListener listener) { timeoutErrorListeners.add(new ListenerRegistration<>(listener, executor)); log.info("timeout error listener added: {}, {}", timeoutErrorListeners.size(), getAddress()); } public boolean removeTimeoutErrorListener(TimeoutErrorListener listener) { - log.info("timeout error listener removed: {}, {}", timeoutErrorListeners.size() - 1, getAddress()); - return ListenerRegistration.removeFromList(listener, timeoutErrorListeners); + boolean removed = ListenerRegistration.removeFromList(listener, timeoutErrorListeners); + if (removed) { + log.info("timeout error listener removed: {}, {}", timeoutErrorListeners.size(), getAddress()); + } else { + log.info("timeout error listener not found for removal: {}", getAddress()); + } + return removed; }The rest of the listener infrastructure (use of
CopyOnWriteArrayListandListenerRegistration, and asynchronous dispatch inqueueTimeoutErrorListeners) looks good.
🧹 Nitpick comments (3)
core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java (1)
861-890: Null-guard ingetHashModifieris correct; document nullable contract and verify callsitesThe new
workBlock == nullcheck cleanly avoids NPEs when the ancestor isn’t in the store yet during sync, which aligns with the added guards inQuorumState.computeQuorumMembers.Since
getHashModifiernow explicitly returnsnullin this situation, consider:
- Marking
getHashModifieras@Nullable(and updating any Javadoc) to make the contract clear.- Quickly scanning other callsites beyond
QuorumState.computeQuorumMembersto ensure they either handlenullor are updated accordingly.core/src/main/java/org/bitcoinj/evolution/QuorumState.java (1)
195-207: Defensive null handling incomputeQuorumMemberslooks good; confirm caller expectations and consider small refinementsThe new early returns when
workBlockor the hash modifier arenullsensibly prevent crashes when the ancestor block or CL-based modifier is unavailable during sync.A couple of follow-ups to consider:
computeQuorumMembers/getAllQuorumMemberscan now returnnullin more situations (on top of the existingallMns == nullpath). Please confirm all callers treatnullas “cannot compute quorum” and don’t blindly iterate the result.- For clarity, you might document the distinction between:
- “No quorum at this height” → empty list (
[]), and- “Quorum cannot be computed (state incomplete)” →
null.- Minor:
workBlockis computed here, andgetHashModifierinternally recomputes the same ancestor; if this is on a hot path, consider passingworkBlockdown or refactoring to avoid the redundant lookup.core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (1)
125-143: Remove unusedcurrentThreadvariable intimeoutOccurred
Thread currentThread = Thread.currentThread();is never used and will trigger IDE/compiler warnings. You can safely drop it without changing behavior:- protected void timeoutOccurred() { - Thread currentThread = Thread.currentThread(); + protected void timeoutOccurred() { log.info("{}: Timed out", getAddress());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java(5 hunks)core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/QuorumState.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListDiff.java(1 hunks)core/src/main/java/org/bitcoinj/quorums/SimplifiedQuorumList.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/org/bitcoinj/quorums/SimplifiedQuorumList.java
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (2)
core/src/main/java/org/bitcoinj/net/NioClient.java (1)
NioClient(31-126)core/src/main/java/org/bitcoinj/net/NioClientManager.java (1)
NioClientManager(36-196)
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
🔇 Additional comments (3)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListDiff.java (1)
207-209: Multi-linetoString()output may affect log consumersThe added
%n %s -> %smakestoString()return a multi-line string. That’s likely fine and improves readability, but be aware it can impact any tooling or tests that expect single-line log entries or parsetoString()output line-by-line. If any such consumers exist, consider keeping it single-line or adding a dedicated, more verbose debug method instead.core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (2)
21-42: Timeout listener imports and threading primitives look appropriateUsing
TimeoutError/TimeoutErrorListenerplusListenerRegistrationwithCopyOnWriteArrayListandExecutormatches the existing listener patterns and gives you safe concurrent iteration and dispatch. No issues here.
325-335: Timeout listener dispatch pattern looks solid
queueTimeoutErrorListenerscorrectly logs the queueing event and uses each listener’s configuredExecutorto invokeonTimeout(error, peer)without holding any locks or touching socket state. This matches the listener patterns elsewhere in the codebase and should be safe under concurrent add/remove operations thanks to theCopyOnWriteArrayList.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (2)
124-144: Consolidate stack trace scan and align blockstore timeout detection with intent
checkForBlockStoreTimeout()currently:
- Calls
Thread.getAllStackTraces()twice and walks stacks twice, which is unnecessary overhead on every timeout.- Only checks for
"peekByteArray"even though comments/logs mention “peekByteArray or SPVBlockStore operations”, so classification and log text can drift from actual detection.- Uses one pass mainly for logging and a second pass to drive the boolean return, instead of a single pass that both logs and determines
true/false.You can simplify and make behavior match the comments by collecting stacks once, scanning them once, and using the same predicate both for logging and for the boolean result. For example:
- private boolean checkForBlockStoreTimeout() { - Thread.getAllStackTraces().forEach((thread, stackTrace) -> { - String threadName = thread.getName(); - if (threadName.contains("PeerGroup Thread") || threadName.contains("NioClientManager")) { - log.warn("Stack trace for thread '{}' (State: {}):", threadName, thread.getState()); - - boolean foundBlockingCall = false; - for (StackTraceElement element : stackTrace) { - log.warn(" at {}", element); - - // Check if this thread is stuck in native peekByteArray or SPVBlockStore operations - String elementStr = element.toString(); - if (elementStr.contains("peekByteArray")) { - foundBlockingCall = true; - } - } - - if (foundBlockingCall) { - log.error("CRITICAL: Thread '{}' is stuck in native I/O operation (peekByteArray/SPVBlockStore)", threadName); - } - } - }); - - // Check all threads for blocking SPVBlockStore calls - for (Thread thread : Thread.getAllStackTraces().keySet()) { - for (StackTraceElement element : thread.getStackTrace()) { - String elementStr = element.toString(); - if (elementStr.contains("peekByteArray")) { - log.error("CRITICAL: Detected SPVBlockStore timeout - native I/O freeze detected in thread: {}", thread.getName()); - return true; - } - } - } - return false; - } + private boolean checkForBlockStoreTimeout() { + Map<Thread, StackTraceElement[]> allStacks = Thread.getAllStackTraces(); + boolean hasBlockStoreTimeout = false; + + for (Map.Entry<Thread, StackTraceElement[]> entry : allStacks.entrySet()) { + Thread thread = entry.getKey(); + StackTraceElement[] stackTrace = entry.getValue(); + String threadName = thread.getName(); + + boolean interestingThread = threadName.contains("PeerGroup Thread") || threadName.contains("NioClientManager"); + if (interestingThread) { + log.warn("Stack trace for thread '{}' (State: {}):", threadName, thread.getState()); + } + + boolean foundBlockingCall = false; + for (StackTraceElement element : stackTrace) { + if (interestingThread) { + log.warn(" at {}", element); + } + String elementStr = element.toString(); + if (elementStr.contains("peekByteArray") + || elementStr.contains("SPVBlockStore.get") + || elementStr.contains("DirectByteBuffer.get")) { + foundBlockingCall = true; + } + } + + if (foundBlockingCall) { + hasBlockStoreTimeout = true; + log.error("CRITICAL: Detected native I/O freeze (peekByteArray/SPVBlockStore/DirectByteBuffer) in thread: {}", threadName); + } + } + + return hasBlockStoreTimeout; + }You’ll also need to add
import java.util.Map;at the top.This keeps the timeout classification logic in one place, avoids the duplicate global stack snapshot, and ensures the return value matches the conditions mentioned in the comments/log messages.
Also applies to: 146-185
314-332: Fix copy‑pasted Javadoc for timeout error listenersThe Javadoc on
addTimeoutErrorListenerstill says “called immediately before a message is received”, which is misleading for a timeout error listener API.Consider updating it to describe the actual behavior, e.g.:
- /** Registers a listener that is called immediately before a message is received */ + /** Registers a listener that will be notified when this peer experiences a socket timeout. */This makes the API self‑documenting for anyone discovering it from
PeerSocketHandler.
🧹 Nitpick comments (1)
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (1)
196-220: Consider using DEBUG level for detailed timing logs.The detailed timing breakdown is logged at INFO level, which will generate a log entry on every wallet write. This might be verbose in production environments. Consider using DEBUG level for these performance diagnostics while keeping high-level timing at INFO if needed.
Apply this diff to adjust the log level:
- log.info("writeWallet timing: {}ms total ({}ms/{}% serialization, {}ms/{}% I/O) buffer={}KB", + log.debug("writeWallet timing: {}ms total ({}ms/{}% serialization, {}ms/{}% I/O) buffer={}KB", totalMs, serializationMs, serializationPercent, ioMs, ioPercent, bufferSize/1024);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java(5 hunks)core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java(3 hunks)
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (1)
21-23: Timeout error listener wiring and types look consistentThe new imports and the
CopyOnWriteArrayList<ListenerRegistration<TimeoutErrorListener>>field are appropriate for the listener pattern used elsewhere in bitcoinj and are thread‑safe for concurrent registration and dispatch.Also applies to: 28-28, 41-42, 67-68
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (2)
148-149: Good fix for backward compatibility.This correctly addresses the past review comment by disabling adaptive sizing when an explicit buffer size is set, preserving the legacy contract for existing integrators.
164-189: TheWallet.getKeyChainGroupSize()method exists and is properly defined in the Wallet class as a public method (line 853 ofcore/src/main/java/org/bitcoinj/wallet/Wallet.java). The method is documented and used throughout the codebase, so the call in thecalculateOptimalBufferSizemethod will execute without runtime errors.
HashEngineering
force-pushed
the
fix/sync-issues-coinjoin
branch
from
d679251 to
5c79026
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 (1)
core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java (1)
860-890: Null-handling gaps ingetHashModifiercall sites require critical fixesThe new null guard in
getHashModifiercorrectly prevents NPE during sync, but three of four call sites do not handle the nullable return, creating NPE risks when the ancestor block is unavailable:
- QuorumRotationState.java:685 (buildNewQuorumQuarterMembers):
modifieris passed directly tocalculateQuorum()at lines 735 and 737 without null check- QuorumRotationState.java:835:
modifieris passed directly tocalculateQuorum()at lines 841–842 without null check- QuorumRotationState.java:913 (getMNUsageBySnapshot):
modifieris passed directly tocalculateQuorum()at line 927 without null checkAdd null checks at these three sites before using
modifier, or add@Nullableannotation to the method return type to document the contract clearly.
♻️ Duplicate comments (3)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
2590-2596: Remove deadhardSaveOnNextBlockstate and commented conditional innotifyNewBestBlock().The
hardSaveOnNextBlockblock here is fully commented out, but the flag is still declared (private boolean hardSaveOnNextBlock = false;) and written inreceive()(hardSaveOnNextBlock = true;), leaving unused state and misleading logic.Within this hunk, drop the commented conditional so the method always coalesces via
saveLater():- //if (hardSaveOnNextBlock) { - // saveNow(); - // hardSaveOnNextBlock = false; - //} else { // Coalesce writes to avoid throttling on disk access when catching up with the chain. saveLater(); - //}Additionally (outside this hunk), remove:
- The field
private boolean hardSaveOnNextBlock = false;(around Line 2329).- The assignment
hardSaveOnNextBlock = true;at the end ofreceive(...)(around Line 2481).After this cleanup, all persistence is consistently driven by
saveLater()in this path.
853-870: Fix lock ordering and synchronize access tokeyChainExtensionsingetKeyChainGroupSize().This method holds only
keyChainGroupLockbut iterateskeyChainExtensions, which is mutated under the walletlock(e.g. inaddExtension/deserializeExtension). That violates the documented lock ordering “lock>keyChainGroupLock” (Lines 134–137) and risks data races/ConcurrentModificationException.Acquire
lockfirst, thenkeyChainGroupLock, and drop the unnecessaryAtomicInteger:public int getKeyChainGroupSize() { - keyChainGroupLock.lock(); - try { - AtomicInteger walletKeys = new AtomicInteger(keyChainGroup.numKeys()); - - if (receivingFromFriendsGroup != null) { - walletKeys.addAndGet(receivingFromFriendsGroup.numKeys()); - } - - if (sendingToFriendsGroup != null) { - walletKeys.addAndGet(sendingToFriendsGroup.numKeys()); - } - - keyChainExtensions.values().forEach(keyChainGroupExtension -> - walletKeys.addAndGet(keyChainGroupExtension.numKeys()) - ); - return walletKeys.get(); - } finally { - keyChainGroupLock.unlock(); - } + lock.lock(); + try { + keyChainGroupLock.lock(); + try { + int walletKeys = keyChainGroup.numKeys(); + + if (receivingFromFriendsGroup != null) { + walletKeys += receivingFromFriendsGroup.numKeys(); + } + + if (sendingToFriendsGroup != null) { + walletKeys += sendingToFriendsGroup.numKeys(); + } + + for (KeyChainGroupExtension ext : keyChainExtensions.values()) { + walletKeys += ext.numKeys(); + } + return walletKeys; + } finally { + keyChainGroupLock.unlock(); + } + } finally { + lock.unlock(); + } }core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java (1)
124-144: Timeout cause detection still does duplicate stack scans and only matchespeekByteArray
checkForBlockStoreTimeout()callsThread.getAllStackTraces()twice and re‑walks all stacks, which is unnecessarily expensive and noisy, especially when many threads exist.- Despite comments/logs mentioning “SPVBlockStore operations”, the detection only checks for
"peekByteArray"; the error log message ("(peekByteArray/SPVBlockStore)") is therefore misleading.- The local
currentThreadintimeoutOccurred()is unused.Consider:
- Calling
Thread.getAllStackTraces()once, logging and computing the boolean in a single pass, and- Either broadening the match (e.g. include
SPVBlockStore/other native I/O indicators as intended) or updating comments/log strings to reflect that you’re only keying offpeekByteArray,- Dropping the unused
currentThreadvariable.This keeps behavior while reducing overhead and making the logs match the actual detection.
Also applies to: 146-185
🧹 Nitpick comments (8)
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (1)
95-220: Adaptive buffer sizing + explicit override behavior looks soundThe combination of:
useAdaptiveBufferSizingdefaulting to true,setWalletWriteBufferSize(...)automatically disabling adaptivity, andsetUseAdaptiveBufferSizing(boolean)to re‑enable itpreserves legacy “explicit buffer wins” semantics while introducing a safe, bounded 8/16/32/64 KB heuristic via
calculateOptimalBufferSize(...). The updatedwriteWalletpath correctly separates serialization and I/O timing, uses the computed buffer withCodedOutputStream, and maintains the existing exception contract.One minor consideration: the new
log.info("writeWallet timing: ...")on every save could be noisy for applications that persist frequently; consider downgrading todebugor gating behindlog.isDebugEnabled()if log volume becomes an issue.- log.info("writeWallet timing: {}ms total ({}ms/{}% serialization, {}ms/{}% I/O) buffer={}KB", - totalMs, serializationMs, serializationPercent, ioMs, ioPercent, bufferSize/1024); + if (log.isDebugEnabled()) { + log.debug("writeWallet timing: {}ms total ({}ms/{}% serialization, {}ms/{}% I/O) buffer={}KB", + totalMs, serializationMs, serializationPercent, ioMs, ioPercent, bufferSize / 1024); + }core/src/main/java/org/bitcoinj/store/FlatDB.java (2)
29-135: Clarify interaction betweensetIOBufferSizeand adaptive sizingThe adaptive
calculateOptimalBufferSize(long dataSize)logic is reasonable and the 4/16/64/128 KB tiers are conservative. However, withuseAdaptiveBufferSizingdefaulting to true, a caller that invokessetIOBufferSize(...)alone will not actually get that size at runtime unless they also callsetUseAdaptiveBufferSizing(false); the adaptive path will still pick its own tier.For consistency with
WalletProtobufSerializer.setWalletWriteBufferSize(...)and to make the API more intuitive, consider automatically disabling adaptive sizing when an explicit buffer is set:public void setIOBufferSize(int bufferSize) { - this.ioBufferSize = bufferSize; + this.ioBufferSize = bufferSize; + this.useAdaptiveBufferSizing = false; }Callers that want adaptivity back can still re‑enable it with
setUseAdaptiveBufferSizing(true).
163-224: Buffered I/O integration preserves behavior and improves performanceSwitching both
writeandreadto useBufferedOutputStream/BufferedInputStreamwith the newcalculateOptimalBufferSize(...)keeps the existing on‑disk format intact while reducing syscall overhead on larger cache files. The try‑with‑resources blocks also tighten resource handling.If you touch this code again, you might consider using
DataInputStream.readFully(...)(or manually looping onread) instead of assuming a singlereadfillsvchDataandhashIn, but that limitation was already present before the buffering change.core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (3)
46-119: Verbose setup and log formatting in test harnessSwitching to
BriefLogFormatter.initVerbose()and adding the custominfo/error/formatMessagehelpers makes the perf tests much easier to inspect, and all usage of{}/{:.xf}placeholders is handled safely for the current calls. Just be aware this greatly increases console output when running the test suite; if CI runtimes or log volume become an issue, consider downgrading some of these to use the standard SLF4J logger or guarding the custom logging behind a verbosity flag.
140-332: Comprehensive buffer-size perf benchmark; consider treating as a “slow” test
walletSavePerformanceTestprovides a very detailed comparison of fixed buffer sizes vs adaptive sizing (including warmups, multiple runs, relative-speed bars, and on-disk file tests). It’s excellent for tuning and regression analysis, but it runs a large number of serializations and emits a lot of log output.If this starts to impact CI time or noise, it might be worth:
- annotating it or moving it to a dedicated perf suite (e.g., using a separate JUnit category), or
- reducing the number of runs / buffer sizes when not in a dedicated performance run.
Functionally, the logic is sound and does not affect production code paths.
417-570: Robust save/load consistency and caching test
walletConsistencyAndCachingPerformanceTestpluscompareWalletsgives strong coverage for:
- binary stability across repeated save/load cycles (including memos, exchange rates, cached values, and coinjoin types), and
- performance gains from protobuf caching on subsequent saves.
The transaction comparison via TxID map avoids ordering issues from the underlying sets, and the assertions on counts/balances are appropriate. Similar to the other perf test, this is relatively heavy and verbose but technically solid; you may eventually want to mark it as a “slow” or perf‑focused test if suite duration grows.
core/src/main/java/org/bitcoinj/core/PeerGroup.java (2)
1299-1351: Reflection on Guava’sListeningScheduledExecutorServiceis brittle; prefer an explicit delegate referenceThe shutdown diagnostics in
stopAsync()are useful, but relying on:java.lang.reflect.Field delegateField = executor.getClass().getDeclaredField("delegate");to reach a
ScheduledThreadPoolExecutorinside Guava’s listening decorator ties you to a private field name and implementation detail. A Guava upgrade or differentListeningScheduledExecutorServiceimplementation would make this path silently degrade (aside from the logged message).Consider instead holding a direct reference to the underlying
ScheduledThreadPoolExecutorwhen you construct the executor (e.g., store it in a separate field) and logging against that, avoiding reflection entirely. The existingtry/catchmakes this non‑fatal today, but the code will be more robust and maintainable with an explicit delegate.
1393-1437:forceStopworks but job descriptions may rarely match actual queued tasks
forceStop(int waitMsBeforeShutdown)correctly:
- Triggers
stopAsync()when still running,- Waits up to
waitMsBeforeShutdown,- Falls back to
shutdownNow()and logs remaining jobs.However,
getJobDescription(Runnable job)relies onjob == triggerConnectionsJobandinstanceof ChainDownloadSpeedCalculator. In practice, the underlyingScheduledThreadPoolExecutorqueues its ownRunnableScheduledFuturewrappers, so these identity/type checks are unlikely to ever match, and you’ll usually log “Unknown task”.If the extra classification is important, you may want to derive descriptions from
job.toString()patterns only, or avoid over‑promising detail in the log messages (“Remaining job” without a semantic label is still useful). Otherwise the method is harmless but not very informative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/src/main/java/org/bitcoinj/core/PeerGroup.java(14 hunks)core/src/main/java/org/bitcoinj/core/PeerSocketHandler.java(5 hunks)core/src/main/java/org/bitcoinj/core/listeners/TimeoutError.java(1 hunks)core/src/main/java/org/bitcoinj/core/listeners/TimeoutErrorListener.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/QuorumState.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListDiff.java(1 hunks)core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java(1 hunks)core/src/main/java/org/bitcoinj/quorums/SimplifiedQuorumList.java(1 hunks)core/src/main/java/org/bitcoinj/store/FlatDB.java(5 hunks)core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(2 hunks)core/src/main/java/org/bitcoinj/wallet/WalletEx.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/WalletFiles.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java(3 hunks)core/src/test/java/org/bitcoinj/core/PeerGroupTest.java(1 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(4 hunks)examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- core/src/main/java/org/bitcoinj/utils/BriefLogFormatter.java
- core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java
- core/src/main/java/org/bitcoinj/quorums/SimplifiedQuorumList.java
- core/src/test/java/org/bitcoinj/core/PeerGroupTest.java
- examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java
- core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListDiff.java
- core/src/main/java/org/bitcoinj/core/listeners/TimeoutErrorListener.java
- core/src/main/java/org/bitcoinj/wallet/WalletFiles.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T22:12:09.613Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 286
File: core/src/main/java/org/bitcoinj/wallet/Wallet.java:6371-6373
Timestamp: 2025-09-18T22:12:09.613Z
Learning: In bitcoinj's Wallet.java, methods returning List<ECKey> can safely return new LinkedList<>(detkeys) where detkeys is List<DeterministicKey>, because DeterministicKey extends ECKey and Java's type inference handles this covariance correctly during list construction.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.javacore/src/main/java/org/bitcoinj/wallet/WalletEx.javacore/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java
🧬 Code graph analysis (2)
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
core/src/main/java/org/bitcoinj/core/TransactionConfidence.java (1)
TransactionConfidence(66-714)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
core/src/main/java/org/bitcoinj/core/Sha256Hash.java (1)
Sha256Hash(38-315)
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
616-633: Thread-safe guard aroundmyUnspentsiteration looks correctTaking the wallet lock for
countInputsWithAmountaligns this method with the rest ofWalletEx’s concurrency model, avoiding concurrent modification ofmyUnspents. Logic and behavior are otherwise unchanged.core/src/main/java/org/bitcoinj/core/listeners/TimeoutError.java (1)
1-6: Timeout error enum is simple and extensibleDefining
TimeoutErrorwithBLOCKSTORE_MEMORY_ACCESSandUNKNOWNprovides a clear, type-safe channel for timeout diagnostics and leaves room to extend with more specific causes later.core/src/main/java/org/bitcoinj/evolution/QuorumState.java (1)
183-211: Null-safety improvements incomputeQuorumMembersverifiedThe new guards for
workBlock == nullandmodifier == nullcorrectly turn previously fatal conditions into logged early-returns. The only call site (SimplifiedQuorumList.java:430) already handles null results defensively with an explicit null-check and warning log. The nullable return contract is preserved and properly maintained throughout the call chain.core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
130-201: CoinJoin extension serialization enhancements validatedThe reworked
serializeWalletExtensionmethod properly:
- cleanly separates and times key-chain, outpoint-rounds cache, salt handling, and protobuf I/O
- persists
WalletEx.mapOutpointRoundsCacheinto theOutpointRoundsCacheproto while the wallet lock is held (viaWallet.saveToFileStream→writeWallet→walletToProto→populateExtensions)- uses a sized
ByteArrayOutputStreamplusCodedOutputStreamwithmax(64KB, serializedSize/4)for efficient writeswhile
deserializeWalletExtensionrestores the cache and coinjoin salt. The wallet's main lock is held throughout the serialization chain, preventing any race with cache updates. This design aligns with the principle thatmapOutpointRoundsCachepersists across normal operations and is only cleared on reorg/reset.Optional: Consider gating the detailed timing log at info level behind
log.isDebugEnabled()to reduce verbosity in production saves, though the current approach is acceptable for diagnostic purposes.core/src/main/java/org/bitcoinj/core/PeerGroup.java (1)
2112-2115: ConfigurablemaxStallsand updated stall handling look coherentThe new
maxStallsfield,setMaxStalls(int)/getMaxStalls(), and the integration intoChainDownloadSpeedCalculator.calculate()make the stall behavior tunable and behave as described:
maxStalls <= 0disables stall disconnections (logging only).- Each time a stall is detected,
maxStallsis decremented under the outerlock, and when it reaches zero, you stop disconnecting peers and log that the network appears slower than the threshold.- Using
getMaxStalls()for the read and a locked decrement keeps this reasonably thread‑safe given how the calculator runs.This matches the Javadoc semantics and doesn’t introduce obvious race or off‑by‑one issues.
Also applies to: 2137-2165, 2219-2261, 2270-2291
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/test/java/org/bitcoinj/core/PeerGroupTest.java (1)
903-913: Flaky timing assertion for 50ms wait due to low timer resolution.As noted in the previous review,
System.currentTimeMillis()has platform-specific variations, "with Windows typically providing 15.625ms granularity while Mac and Linux systems achieve ~1ms precision." For a 50ms wait, this can cause the assertion>= 50to occasionally fail.The past review suggested using
System.nanoTime()withTimeUnit.MILLISECONDS.toNanos()for reliable sub-100ms timing assertions.
🧹 Nitpick comments (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
539-542: PreferassertNotNulloverassertEquals(..., true, tx2 != null).The current pattern is verbose and less idiomatic.
assertNotNullprovides clearer semantics and better failure messages.- // Verify the transaction exists in wallet2 - assertEquals("Transaction " + tx1.getTxId() + " should exist in wallet #" + wallet2Number, - true, tx2 != null); + // Verify the transaction exists in wallet2 + assertNotNull("Transaction " + tx1.getTxId() + " should exist in wallet #" + wallet2Number, tx2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/test/java/org/bitcoinj/core/PeerGroupTest.java(1 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/test/java/org/bitcoinj/core/PeerGroupTest.java (1)
888-901: LGTM!The test correctly validates
forceStopbehavior when the PeerGroup is running. The 10-second threshold is sufficiently large thatSystem.currentTimeMillis()resolution differences across platforms won't cause flakiness.core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (3)
47-59: LGTM!The setup method changes appropriately enable verbose logging for debugging performance tests and load the updated wallet file.
72-76: LGTM!The expected balance update aligns with loading a different wallet file (
coinjoin-cache.wallet) in the setup method.
99-107: LGTM!The switch to
toString(false, false, true, null)consistently tests the non-CoinJoin variant for both initial and subsequent calls.
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/src/main/java/org/bitcoinj/core/PeerGroup.java (3)
163-164: TimeoutErrorListener wiring looks consistent with existing listener patternsThe new
timeoutErrorListenerscollection plusaddTimeoutErrorListener,removeTimeoutErrorListener, early registration inconnectTo(...), cleanup inhandlePeerDeath(...), andqueueTimeoutErrorListeners(...)all mirror how other listener types are managed (blocks, headers, masternode list, etc.). The lifecycle and thread‑safety viaCopyOnWriteArrayListand per‑peer add/remove calls look sound.If you anticipate external users registering timeout listeners frequently, you might consider adding a convenience overload
addTimeoutErrorListener(TimeoutErrorListener listener)that defaults toThreading.USER_THREAD, in line with the other listener APIs, but this is optional and not required for correctness.Also applies to: 915-921, 1034-1042, 1725-1727, 2089-2090, 2456-2460
1298-1377: Executor shutdown diagnostics are useful but rely on Guava internals; consider holding the delegate directlyThe extra logging in
stopAsync()andforceStop()(reflection to get thedelegateScheduledThreadPoolExecutor, queue dumps, thread stack traces, and job labeling viagetJobDescription) will be valuable when debugging hangs during shutdown. Two improvement points:
- The reflection on
executor.getClass().getDeclaredField("delegate")reaches into Guava’s private implementation details. Although you safely catch exceptions, this is brittle across Guava upgrades. Since you create theScheduledThreadPoolExecutoryourself increatePrivateExecutor(), you could store it in a dedicated field (e.g.,private final ScheduledThreadPoolExecutor executorDelegate) at creation time and use that directly here, avoiding reflection entirely.- Enumerating all threads and logging stack traces at
INFOmight be quite noisy in environments with many threads. You may want to guard that part behind a debug flag orlog.isDebugEnabled()check so production logs stay cleaner while retaining the tool for investigations.These are maintainability/observability refinements; the current behavior is functionally safe because failures in the reflective path are contained to logging.
Also applies to: 1392-1436
2109-2112: ConfigurablemaxStallsworks, but contract around values could be tightenedThe introduction of
@GuardedBy("lock") int maxStalls, plussetMaxStalls(int)/getMaxStalls()and the use of this value inChainDownloadSpeedCalculator.calculate()to decrement and eventually stop stall‑based disconnects, gives a clear and tunable behavior:
maxStalls > 0: limited number of stall‑triggered disconnects, with the log message showing remaining stalls.maxStalls <= 0: treated as “stall disconnects disabled” (currentMaxStalls <= 0path).Two minor points you may want to refine:
- The Javadoc on
setMaxStallssays “0 to disable stall disconnections entirely”, but negative values are also effectively treated as disabled. Either clamp negatives to 0 or document that “<= 0 disables stall disconnects” to keep the contract precise.ChainDownloadSpeedCalculatordecrementsPeerGroup.this.maxStallsunderlockwhile operating under its ownsynchronized (this)block. That ordering (firstsynchronized(this)incalculate, then acquiringPeerGroup.lock) is only one‑way and, as written, doesn’t have any corresponding code that acquiresPeerGroup.lockand then synchronizes on the calculator, so there’s no immediate deadlock risk. If you later add such code, preserve this lock ordering.Overall, the new stall control logic looks correct and should help avoid overly aggressive disconnects on persistently slow networks.
Also applies to: 2134-2162, 2258-2275, 2288-2289
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/org/bitcoinj/core/PeerGroup.java(13 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(4 hunks)core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
📚 Learning: 2025-09-18T22:12:09.613Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 286
File: core/src/main/java/org/bitcoinj/wallet/Wallet.java:6371-6373
Timestamp: 2025-09-18T22:12:09.613Z
Learning: In bitcoinj's Wallet.java, methods returning List<ECKey> can safely return new LinkedList<>(detkeys) where detkeys is List<DeterministicKey>, because DeterministicKey extends ECKey and Java's type inference handles this covariance correctly during list construction.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.javacore/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
core/src/test/java/org/bitcoinj/store/WalletProtobufSerializerTest.java (1)
163-163: Add explanatory comment documenting the expected key composition.The key count of 18 is mathematically correct for the test setup:
- Lookahead zone: (lookaheadSize + threshold) × 2 chains = (5 + 1) × 2 = 12 keys
- Hierarchy keys: seed (1) + master (1) + account (1) + parent keys (2) = 5 keys
- Imported key: 1 key
- Total: 18 keys
For the UNITTEST network,
KeyChainGroupdefaults tolookaheadSize=5(unit tests use reduced settings). The change from 6 to 18 reflects keys now being counted correctly or the lookahead configuration being applied during serialization.Recommendation: Add a comment to document this calculation:
// Expected: (lookaheadSize=5 + threshold=1) * 2 chains + hierarchy keys (5) + imported key (1) = 18 total assertEquals(18, walletProto.getKeyCount());This makes the assertion self-documenting and ensures future maintainers understand why 18 is correct.
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
855-870: ReworkedgetKeyChainGroupSizenow respects lock ordering and avoids races.This implementation acquires
lockand thenkeyChainGroupLockand uses a simpleintaccumulator instead ofAtomicInteger, which aligns with the documented lock ordering and avoids concurrent iteration issues overkeyChainExtensions. Just ensure callers do not already holdkeyChainGroupLockwithoutlock, to avoid potential lock-order inversions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (4)
249-253: Potential division by zero if elapsed time is 0 ms.If
originalTimeis 0 (possible on very fast systems or due to timing granularity), these divisions will produceInfinityorNaN. Consider adding a guard.- double improvementOverOriginal = ((double)(originalTime - adaptiveAvg) / originalTime) * 100; - double improvementOverBest = ((double)(bestFixedTime - adaptiveAvg) / bestFixedTime) * 100; - - info("Adaptive vs Original 4KB: {:.1f}% improvement", improvementOverOriginal); - info("Adaptive vs Best Fixed: {:.1f}% improvement", improvementOverBest); + if (originalTime > 0) { + double improvementOverOriginal = ((double)(originalTime - adaptiveAvg) / originalTime) * 100; + info("Adaptive vs Original 4KB: {:.1f}% improvement", improvementOverOriginal); + } + if (bestFixedTime > 0) { + double improvementOverBest = ((double)(bestFixedTime - adaptiveAvg) / bestFixedTime) * 100; + info("Adaptive vs Best Fixed: {:.1f}% improvement", improvementOverBest); + }
313-314: Same division by zero risk with file I/O timing.If the file save completes in under 1 ms, this calculation will fail.
- double fileImprovementPercent = ((double) (originalFileWatch.elapsed().toMillis() - adaptiveFileWatch.elapsed().toMillis()) / originalFileWatch.elapsed().toMillis()) * 100; - info("File I/O improvement: {:.1f}%", fileImprovementPercent); + long originalFileMs = originalFileWatch.elapsed().toMillis(); + if (originalFileMs > 0) { + double fileImprovementPercent = ((double) (originalFileMs - adaptiveFileWatch.elapsed().toMillis()) / originalFileMs) * 100; + info("File I/O improvement: {:.1f}%", fileImprovementPercent); + }
539-541: UseassertNotNullfor null checks.
assertEquals(true, tx2 != null)is awkward and produces less helpful failure messages thanassertNotNull.- assertEquals("Transaction " + tx1.getTxId() + " should exist in wallet #" + wallet2Number, - true, tx2 != null); + assertNotNull("Transaction " + tx1.getTxId() + " should exist in wallet #" + wallet2Number, tx2);This requires adding
import static org.junit.Assert.assertNotNull;or using the fully qualified call.
548-559: Exchange rate comparison logic is correct but could be clearer.The nested conditionals work correctly, but the else branch assertion message is slightly misleading. Consider simplifying:
- if (tx1.getExchangeRate() == null) { - assertEquals("Transaction " + tx1.getTxId() + " exchange rate should be null in both wallets", - null, tx2.getExchangeRate()); - } else if (tx2.getExchangeRate() != null) { - assertEquals("Transaction " + tx1.getTxId() + " exchange rate should match between wallets", - tx1.getExchangeRate().coin, tx2.getExchangeRate().coin); - assertEquals("Transaction " + tx1.getTxId() + " exchange rate fiat should match between wallets", - tx1.getExchangeRate().fiat, tx2.getExchangeRate().fiat); - } else { - assertEquals("Transaction " + tx1.getTxId() + " exchange rate should not be null in wallet #" + wallet2Number, - tx1.getExchangeRate(), tx2.getExchangeRate()); - } + if (tx1.getExchangeRate() == null) { + assertNull("Transaction " + tx1.getTxId() + " exchange rate should be null in wallet #" + wallet2Number, + tx2.getExchangeRate()); + } else { + assertNotNull("Transaction " + tx1.getTxId() + " exchange rate should not be null in wallet #" + wallet2Number, + tx2.getExchangeRate()); + assertEquals("Transaction " + tx1.getTxId() + " exchange rate coin should match", + tx1.getExchangeRate().coin, tx2.getExchangeRate().coin); + assertEquals("Transaction " + tx1.getTxId() + " exchange rate fiat should match", + tx1.getExchangeRate().fiat, tx2.getExchangeRate().fiat); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java(1 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
🔇 Additional comments (4)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java (1)
448-464: LGTM! The shutdown logic correctly implements graceful termination with bounded waits.The implementation properly:
- Attempts graceful shutdown first with a 5-second timeout
- Falls back to forced shutdown only after the timeout expires
- Handles
InterruptedExceptioncorrectly by re-setting the interrupt flag- Always calls
saveNow()after shutdown to ensure state persistenceThis addresses the previous review concerns about inverted save logic and potential data loss.
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (3)
47-59: LGTM!The setup method properly loads the test wallet with appropriate exception handling. The switch to
coinjoin-cache.walletand verbose logging aligns with the test's benchmarking purpose.
335-348: LGTM!Clean helper method for visual output in benchmarks.
418-507: LGTM!Good use of
@Ignorefor this expensive benchmark test. The division by zero guard on line 482 is correctly implemented, and the test provides valuable consistency verification.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java (1)
448-454: Make threadPool shutdown graceful (bounded wait) instead of always interrupting tasksThe current sequence
threadPool.shutdown(); // Don't wait at all - let it die naturally to avoid blocking if (!threadPool.isTerminated()) { log.info("ThreadPool shutdown initiated, not waiting"); threadPool.shutdownNow(); // Send interrupt signal but don't wait } saveNow(); // Always save, regardless of thread pool statehas a few problems:
shutdownNow()will run in almost all realistic cases, so running qrinfo tasks are very likely to be interrupted mid‑update.- Those tasks are responsible for
quorumRotationState.processDiff(...),processMasternodeList(...),save(), andopComplete.set(true). Interrupting them can:
- Drop or partially apply the latest quorum/masternode state.
- Leave
opCompletefutures incomplete, potentially hanging callers.- Cause
saveNow()to persist state before in‑flight tasks have finished updating it.- The comment “let it die naturally” contradicts the actual behavior of immediately calling
shutdownNow().Consider switching to a bounded graceful shutdown with a short await before forcing interrupts, and only then calling
saveNow(); for example:- peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener); - threadPool.shutdown(); - // Don't wait at all - let it die naturally to avoid blocking - if (!threadPool.isTerminated()) { - log.info("ThreadPool shutdown initiated, not waiting"); - threadPool.shutdownNow(); // Send interrupt signal but don't wait - } - saveNow(); // Always save, regardless of thread pool state + peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener); + threadPool.shutdown(); + try { + // Wait briefly for in‑flight qrinfo tasks to complete gracefully + if (!threadPool.awaitTermination(5, TimeUnit.SECONDS)) { + log.warn("ThreadPool did not terminate in time, forcing shutdown"); + threadPool.shutdownNow(); + if (!threadPool.awaitTermination(2, TimeUnit.SECONDS)) { + log.error("ThreadPool did not terminate after shutdownNow"); + } + } + } catch (InterruptedException e) { + log.warn("Interrupted while waiting for threadPool termination", e); + threadPool.shutdownNow(); + Thread.currentThread().interrupt(); + } + saveNow(); // Persist final state after tasks have had a chance to finishThis gives ongoing diff processing a fair chance to complete and set their futures, bounds the close-time blocking, and ensures
saveNow()runs after the pool has (mostly) quiesced.core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
268-269:saveOnNextBlockis still racy across threads (same concern as earlier review).
saveOnNextBlockis written insetSaveOnNextBlockwithout any synchronization orvolatile, but read insidenotifyNewBestBlockwhile holdinglock. Without a happens‑before edge, other threads (e.g. UI) toggling this flag are not guaranteed to be seen by the block-processing thread, so “disable saves on next block” can be missed or applied late.The earlier review already suggested guarding this flag with
lock; the code still matches the pre‑fix shape.Recommend aligning with that suggestion:
- // save now on blocks with transactions - private boolean saveOnNextBlock = true; + // save now on blocks with transactions + @GuardedBy("lock") + private boolean saveOnNextBlock = true; @@ - public void setSaveOnNextBlock(boolean saveOnNextBlock) { - this.saveOnNextBlock = saveOnNextBlock; - } - - public boolean getSaveOnNextBlock() { - return saveOnNextBlock; - } + public void setSaveOnNextBlock(boolean saveOnNextBlock) { + lock.lock(); + try { + this.saveOnNextBlock = saveOnNextBlock; + } finally { + lock.unlock(); + } + } + + public boolean getSaveOnNextBlock() { + lock.lock(); + try { + return saveOnNextBlock; + } finally { + lock.unlock(); + } + }No changes are required to
notifyNewBestBlock, as it already runs underlock.Also applies to: 1860-1866, 2607-2613
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java(1 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
📚 Learning: 2025-09-18T22:12:09.613Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 286
File: core/src/main/java/org/bitcoinj/wallet/Wallet.java:6371-6373
Timestamp: 2025-09-18T22:12:09.613Z
Learning: In bitcoinj's Wallet.java, methods returning List<ECKey> can safely return new LinkedList<>(detkeys) where detkeys is List<DeterministicKey>, because DeterministicKey extends ECKey and Java's type inference handles this covariance correctly during list construction.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🔇 Additional comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
860-875: Locking ingetKeyChainGroupSize()now respects wallet/keyChainGroup lock ordering.Acquiring
lockfirst and thenkeyChainGroupLock, and iteratingkeyChainExtensionsonly underlock, fixes the previous lock‑ordering and concurrent‑modification risks while still using the finer-grained keyChainGroupLock for the key-group calls. This aligns with the class comment (“lock > keyChainGroupLock”) and looks correct.
1938-1947: Simplified duplicate‑transaction check inisConsistentOrThrow()looks correct.Computing
expectedSizefrom the pool maps and then comparing togetTransactions(true).size()(which deduplicates byTransaction.equals/hashCodeon txId) is a clean way to detect transactions present in multiple pools. Re‑enteringlockinsidegetTransactions(true)is safe withReentrantLock. No issues here.
| // Index mapping outpoints to transaction hashes that spend them. | ||
| // Used to make findDoubleSpendsAgainst() O(I) instead of O(W × I). | ||
| @GuardedBy("lock") | ||
| private final Map<TransactionOutPoint, Set<Sha256Hash>> spentOutpointsIndex = new HashMap<>(); |
There was a problem hiding this comment.
Keep spentOutpointsIndex consistent when transactions are removed to avoid stale entries and growth.
The index itself and its use in findDoubleSpendsAgainst look solid (O(I) and properly @GuardedBy("lock")), but there are a couple of lifecycle gaps where entries are left behind when transactions are removed:
cleanup()removes risky pending txs frompendingandtransactions, but never callsremoveFromSpentOutpointsIndex(tx), so their outpoints stay inspentOutpointsIndexindefinitely.clearTransactions(int fromHeight)withfromHeight == 0callsclearTransactions()but does not clearspentOutpointsIndex; onlyreset()does so explicitly. After a replay viaclearTransactions(0), the index can still hold outpoints for transactions that were fully dropped.
Functionally, lookups stay correct because every findDoubleSpendsAgainst is filtered through the candidates map, but the index will grow monotonically across cleanups/replays, which is undesirable for long‑running wallets.
I suggest:
- Clear the index in the core
clearTransactions()method so both reset/replay paths stay in sync:
private void clearTransactions() {
unspent.clear();
spent.clear();
pending.clear();
dead.clear();
transactions.clear();
myUnspents.clear();
+ spentOutpointsIndex.clear();
}and then simplify reset():
- clearTransactions();
- spentOutpointsIndex.clear();
+ clearTransactions();- Keep the index up to date when
cleanup()permanently drops a pending tx:
- i.remove();
- transactions.remove(tx.getTxId());
+ i.remove();
+ transactions.remove(tx.getTxId());
+ removeFromSpentOutpointsIndex(tx);This keeps spentOutpointsIndex bounded and makes its invariants (“only wallet-resident transactions are indexed”) match the pool/transactions maps.
Also applies to: 2252-2272, 2274-2286, 2796-2807, 3432-3436, 3544-3548, 3578-3585, 3621-3643
🤖 Prompt for AI Agents
In core/src/main/java/org/bitcoinj/wallet/Wallet.java around lines 170-173,
ensure spentOutpointsIndex is kept consistent when transactions are removed:
modify clearTransactions() to clear spentOutpointsIndex so both replay
(clearTransactions(0)) and normal clear paths drop indexed outpoints; simplify
reset() to rely on clearTransactions() (removing its explicit clear of
spentOutpointsIndex); and in cleanup() call removeFromSpentOutpointsIndex(tx)
whenever a pending transaction is permanently removed from pending/transactions
so stale entries do not accumulate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/main/java/org/bitcoinj/manager/DashSystem.java (3)
255-271: Make close() more robust against exceptions in individual component shutdownsThe added logging makes shutdown sequencing clearer, but as written any exception in one
close(...)call (e.g.,sporkManager.close(peerGroup)) will prevent the remaining managers from being closed and their corresponding log lines from being emitted.Consider wrapping each close in its own
try/catchand logging failures while continuing with the rest, e.g.:- log.info("Closing network spork configuration manager"); - sporkManager.close(peerGroup); + try { + log.info("Closing network spork configuration manager"); + sporkManager.close(peerGroup); + } catch (Exception e) { + log.warn("Error while closing network spork configuration manager", e); + }and applying the same pattern for
masternodeSync,masternodeListManager,instantSendManager,signingManager,chainLockHandler,quorumManager, andcoinJoinManager. This avoids partial shutdowns due to one misbehaving component.
272-275: Defensively guard LLMQ thread interruption and reuse existing sync-flag helperThe new
if (masternodeSync.hasSyncFlag(...))block assumes bothmasternodeSyncandllmqBackgroundThreadare non-null. That is currently guaranteed by the init/closeDash ordering, but it’s a brittle assumption.For extra safety and consistency with
start()/shutdown(), consider:
- Using
getSyncFlags().contains(SYNC_INSTANTSENDLOCKS)(orDashSystem.hasSyncFlag(...)) rather than callingmasternodeSyncdirectly.- Guarding the thread before interrupting, in line with
stopLLMQThread():- if(masternodeSync.hasSyncFlag(MasternodeSync.SYNC_FLAGS.SYNC_INSTANTSENDLOCKS)) { - log.info("Stopping LLMQ background thread"); - llmqBackgroundThread.interrupt(); - } + if (hasSyncFlag(MasternodeSync.SYNC_FLAGS.SYNC_INSTANTSENDLOCKS) + && llmqBackgroundThread != null + && llmqBackgroundThread.isAlive()) { + log.info("Stopping LLMQ background thread"); + llmqBackgroundThread.interrupt(); + }This keeps the behavior aligned with other lifecycle code and is more resilient to future changes in initialization order.
276-289: Align close()/shutdown() behavior for scheduled tasks and clarify PeerGroup lifecycleIn
close()you now:
- Remove the
newBestBlockListener.- Optionally cancel
scheduledMasternodeSync.- Close
blockChain/headerChain.- Clear
peerGroupreference.A couple of points to tighten up:
- Scheduled task cancellation symmetry
shutdown()cancels all three scheduled tasks and nulls them:scheduledMasternodeSync.cancel(false); scheduledMasternodeSync = null; scheduledNetFulfilled.cancel(false); scheduledNetFulfilled = null; scheduledGovernance.cancel(false); scheduledGovernance = null;
close()only cancelsscheduledMasternodeSync, and leavesscheduledNetFulfilledandscheduledGovernanceuntouched. IfcloseDash()is ever used without a priorshutdown(), these two tasks can continue running (and invokingdoMaintenance()on objects you just closed), which is at least a reliability risk.Two possible cleanups:
- Either have
close()callshutdown()first (and makeshutdown()idempotent), or- Mirror the full cancellation here, with logs, e.g.:
- if (scheduledMasternodeSync != null) { - log.info("Cancelling scheduled masternode sync task"); - scheduledMasternodeSync.cancel(true); - } + if (scheduledMasternodeSync != null) { + log.info("Cancelling scheduled masternode sync task"); + scheduledMasternodeSync.cancel(true); + scheduledMasternodeSync = null; + } + if (scheduledNetFulfilled != null) { + log.info("Cancelling scheduled net fulfilled maintenance task"); + scheduledNetFulfilled.cancel(true); + scheduledNetFulfilled = null; + } + if (scheduledGovernance != null) { + log.info("Cancelling scheduled governance maintenance task"); + scheduledGovernance.cancel(true); + scheduledGovernance = null; + }
- PeerGroup lifecycle vs. reference clearing
log.info("Clearing peer group reference"); peerGroup = null;only drops the reference; it doesn’t stop thePeerGroupitself. That may be correct if the caller ownsPeerGrouplifecycle (e.g., callingpeerGroup.stopAsync()/awaitTerminated()elsewhere), but it would be good to double-check that this is indeed guaranteed in all shutdown paths and document that expectation here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/bitcoinj/manager/DashSystem.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/main/java/org/bitcoinj/manager/DashSystem.java (1)
core/src/main/java/org/bitcoinj/core/MasternodeSync.java (1)
MasternodeSync(30-616)
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.