Fix/refresh unused keys#296
Open
HashEngineering wants to merge 13 commits into
Open
Conversation
* don't remove peer listeners, that will be handled by PeerGroup
📝 WalkthroughWalkthroughRefactors shutdown flows to null peerGroup/blockChain and add early-return guards; adds DualBlockChain download-tracker with lifecycle wiring; updates CoinJoin shutdown/logging and mixing-progress algorithm (keeps old as deprecated); adds DumpMasternodeList example and updates CI workflow. ChangesShutdown / Listener lifecycle & nulling
DualBlockChain download tracking & DashSystem wiring
Spork & Quorum state adjustments
CoinJoinExtension mixing-progress & tests
New example utility
CI workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
core/src/main/java/org/bitcoinj/manager/DashSystem.java (1)
255-297:⚠️ Potential issue | 🟠 MajorGuard
dualBlockChain.close()against null initialization.The field
dualBlockChainis declared at line 61 but only assigned insetPeerGroupAndBlockChain()(line 304). However,setMasternodeListManager()(line 156) creates a localDualBlockChainvariable and never assigns it to the field. Ifclose()is called after only usingsetMasternodeListManager(), the call todualBlockChain.close()at line 293 will NPE.Either assign the field in
setMasternodeListManager()by changing line 156 tothis.dualBlockChain = ..., or add a null-check before closing:if (dualBlockChain != null) { dualBlockChain.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/manager/DashSystem.java` around lines 255 - 297, The close() method calls dualBlockChain.close() unguarded which can NPE if dualBlockChain was never assigned (it’s only set in setPeerGroupAndBlockChain() and setMasternodeListManager() currently creates a local DualBlockChain). Fix by either assigning the field in setMasternodeListManager() (change the local instantiation to set this.dualBlockChain = ...) or by adding a null-check in close() before calling dualBlockChain.close() (if (dualBlockChain != null) { dualBlockChain.close(); }), referencing the dualBlockChain field, setMasternodeListManager(), setPeerGroupAndBlockChain(), and close().core/src/main/java/org/bitcoinj/evolution/QuorumRotationState.java (1)
444-468:⚠️ Potential issue | 🟠 MajorTake a local snapshot of
blockChainbefore using it.The
blockChainfield is not volatile andisSynced()does not synchronize. Since shutdown code nullifiesblockChain, a race condition exists whereblockChaincan transition from non-null to null between the checks at lines 449 and the subsequent calls at lines 452, 460. Capturing a local snapshot prevents NPE during concurrent shutdown.🛠️ Suggested fix
+ DualBlockChain currentBlockChain = blockChain; - if (blockChain != null && !params.isDIP0024Active(blockChain.getBestChainHeight())) + if (currentBlockChain != null && !params.isDIP0024Active(currentBlockChain.getBestChainHeight())) return true; if (mnListAtH.getHeight() == -1) return false; - if (blockChain == null) + if (currentBlockChain == null) return false; - if (!blockChain.isInitialHeaderSyncComplete()) { + if (!currentBlockChain.isInitialHeaderSyncComplete()) { return false; } - int mostCommonHeight = blockChain.getBestChainHeight(); + int mostCommonHeight = currentBlockChain.getBestChainHeight();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/evolution/QuorumRotationState.java` around lines 444 - 468, isSynced() races on the non-volatile field blockChain; capture a local snapshot (e.g., BlockChain chain = this.blockChain) at the start of the method and use that local variable for all subsequent calls (chain.getBestChainHeight(), chain.isInitialHeaderSyncComplete(), and passing to params.isDIP0024Active) and for the null checks so the method cannot NPE if shutdown nulls the field concurrently; ensure you return the same results by checking snapshot == null where appropriate before using its methods.core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java (1)
129-137:⚠️ Potential issue | 🟠 MajorCapture
blockChainreference at method entry to prevent concurrent nulling during shutdown.The early null check at line 206 is insufficient. Between the lock release (line 220) and re-acquisition (line 255),
close()can execute and nullblockChainwithout holding the lock. This creates a window where line 269's dereference will fail.🔧 Suggested fix
void processNewChainLock(Peer from, ChainLockSignature clsig, Sha256Hash hash) { - if (blockChain == null) return; // closed during shutdown + AbstractBlockChain currentBlockChain = blockChain; + if (currentBlockChain == null) return; // closed during shutdown lock.lock(); try { @@ -269 +269 @@ - StoredBlock block = blockChain.getBlockStore().get(clsig.blockHash); + StoredBlock block = currentBlockChain.getBlockStore().get(clsig.blockHash);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java` around lines 129 - 137, Capture the current blockChain field into a local final variable at the start of the method that performs the early null check (in class ChainLocksHandler, the method that references blockChain between the early check and the dereference) to prevent close() from nulling the field mid-execution; i.e., create a local variable (e.g., final BlockChain localChain = this.blockChain) immediately after method entry and use localChain for all subsequent null checks and dereferences instead of referencing the field directly, leaving the close() method unchanged.core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java (1)
122-149:⚠️ Potential issue | 🟠 Major
close()must synchronize field nulling with ongoing callbacks or add null guards.
preMessageReceivedEventListeneris intentionally not unregistered inclose()(to avoid deadlock), but remains active and can invokeprocessInstantSendLock(). This method accessesblockChain(viablockChain.getBlockStore().get()andblockChain.getChainHead()) before acquiring thelock, exposing a race withclose()nulling these fields without synchronization. A concurrent callback will hitNullPointerException.Either:
- Acquire the
lockearlier inprocessInstantSendLock()before accessingblockChain, or- Add null checks before field access, or
- Synchronize
close()with pending callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines 122 - 149, The race is that preMessageReceivedEventListener can call processInstantSendLock() which reads blockChain before the instance lock is held while close() can null blockChain; to fix, start processInstantSendLock() by acquiring the existing lock (the same lock used to protect instance state) before any access to blockChain, check if blockChain is null and return if so, and release the lock in a finally block; this ensures callbacks via preMessageReceivedEventListener cannot observe partially-closed state even though close() does not unregister that listener.core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java (1)
288-313:⚠️ Potential issue | 🟠 Major
close()never cancels the scheduled maintenance task before nulling collaborators.This method nulls
blockChainandpeerGroup, but it never cancelsschedule, which meansdoMaintenance()can still be scheduled to run against null fields. Thestop()method properly cancelsscheduleand shuts down the message executor withawaitTermination(), butclose()skips this critical cleanup. If called withoutstop()being called first,doMaintenance()could reference null fields.Additionally,
close()callsexecToStop.shutdown()without waiting for termination, whilestop()properly usesawaitTermination(10, TimeUnit.SECONDS). Reuse the existingstop()logic before clearing shared state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java` around lines 288 - 313, The close() method must cancel the scheduled maintenance and wait for the message executor to terminate before nulling collaborators; update close() to mirror stop() behavior by cancelling the schedule (future returned from schedule), ensuring any scheduled doMaintenance() is cancelled, and invoking the same shutdown + awaitTermination logic used in stop() for messageProcessingExecutor (instead of only shutdown()), then clear blockChain and peerGroup; reference the existing stop(), close(), schedule (the ScheduledFuture), doMaintenance(), and messageProcessingExecutor symbols so the cleanup sequence is performed safely.core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
551-582:⚠️ Potential issue | 🟠 MajorThe precomputed transaction snapshot can resurrect a used key.
wallet.getTransactions(true)is only a snapshot, andprocessTransaction()updatesunusedKeys/keyUsagewithout takingunusedKeysLock. If a transaction arrives after Lines 553-561 but before this rebuild completes,refreshUnusedKeys()can overwrite that newer state and put the just-used key back intounusedKeys, which risks address reuse. This refresh needs the same synchronization as the live transaction updates, or a final revalidation step before committing the rebuilt maps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java` around lines 551 - 582, The rebuild can race with processTransaction and resurrect a just-used key; to fix, ensure the precomputed usedPubKeyHashes is validated while holding the same lock used for committing the rebuilt maps: compute or recompute usedPubKeyHashes after acquiring unusedKeysLock (or, alternatively, after acquiring unusedKeysLock re-scan wallet.getTransactions(true) to add any new outputs) before mutating unusedKeys and keyUsage; keep references to unusedKeysLock, wallet.getTransactions(true), usedPubKeyHashes, unusedKeys, keyUsage, issuedKeys and processTransaction so reviewers can find and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/bitcoinj/core/DualBlockChain.java`:
- Around line 134-155: The close() method currently just nulls peerGroup leaving
downloadProgressTracker registered; update close() to detach
downloadProgressTracker from the existing peerGroup by calling the corresponding
remove...EventListener/remove...Listener methods (the same ones used in
setPeerGroup: removeHeadersDownloadedEventListener,
removeHeadersDownloadStartedEventListener, removeBlocksDownloadedEventListener,
removeChainDownloadStartedEventListener, removeMasternodeListDownloadListener
with Threading.USER_THREAD and the stored downloadProgressTracker) before
nulling peerGroup and clearing downloadProgressTracker; also modify
setPeerGroup(PeerGroup, MasternodeSync) to check for an existing peerGroup +
downloadProgressTracker and remove the old tracker from that peerGroup first (to
avoid duplicate registrations) before assigning the new peerGroup and
creating/adding a fresh MyDownloadProgressTracker.
In `@core/src/main/java/org/bitcoinj/core/SporkManager.java`:
- Around line 107-116: close() currently nulls the PeerGroup reference without
unregistering the PeerGroup-level listener, leaving peerConnectedEventListener
registered and causing future GetSporksMessage calls or duplicate callbacks when
setBlockChain() is called; fix by first calling
peerGroup.removeConnectedEventListener(peerConnectedEventListener) (and nulling
peerConnectedEventListener) before setting peerGroup = null, and also add a
guard in setBlockChain() to avoid re-registering the same
peerConnectedEventListener if it's already attached.
In
`@core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java`:
- Around line 454-457: The shutdown sequence in SimplifiedMasternodeListManager
nulls blockChain before calling saveNow(), causing saveNow() to operate without
the blockchain state; reorder the operations in the close method so you call
saveNow() while blockChain (and peerGroup) are still valid, then set peerGroup =
null and blockChain = null, and finally call super.close(); this ensures
saveNow() persists the correct quorum/list state.
In `@core/src/main/java/org/bitcoinj/governance/GovernanceManager.java`:
- Around line 1718-1721: close() currently only nulls the peerGroup reference
but leaves this manager registered as a PeerGroup listener
(getDataEventListener, preMessageReceivedEventListener), so it can still receive
callbacks; before setting peerGroup = null, explicitly unregister the listeners
by calling peerGroup.removeDataEventListener(getDataEventListener()) and
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener)
(or the corresponding remove* methods present on PeerGroup) and handle/ignore
any exceptions or early-return if peerGroup is already shutting down to avoid
deadlock; after successful removal, then set peerGroup = null so no lingering
references remain.
In `@core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java`:
- Around line 783-795: The partial-progress branch is incorrectly guarded by
"rounds >= 0" causing outputs with negative roundsMixed (from
getRealOutpointCoinJoinRounds()) to be counted and add negative values to
totalMixed; change the condition to check "roundsMixed >= 0" before incrementing
totalInputs and adding percentMixedForInput so only valid per-output progress
contributes to totalInputs/totalMixed (use roundsMixed, requiredRounds,
totalInputs, totalMixed, and the getRealOutpointCoinJoinRounds() result to
locate the logic in CoinJoinExtension).
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java`:
- Around line 95-101: The two info() calls log the wrong getter names for the
measured times — swap the log labels so the Stopwatch stored in watch2 is logged
with "getMixingProgress2" and the Stopwatch in watch3 is logged with
"getMixingProgress"; update the info(...) invocations that follow the assertions
around wallet.getCoinJoin().getMixingProgress2() and
wallet.getCoinJoin().getMixingProgress() accordingly so each info() references
the matching getter name.
In `@examples/src/main/java/org/bitcoinj/examples/DumpMasternodeList.java`:
- Around line 61-68: The switch on the network string in DumpMasternodeList
currently falls through to MainNetParams on unknown input; change it to
explicitly handle only "testnet" and "mainnet" (or "main") and throw an
IllegalArgumentException (or print usage and exit) for any other value so typos
don't silently default to MainNetParams.get(); update the switch (or if/else)
around the variable params and use TestNet3Params.get() and MainNetParams.get()
only for recognized tokens and fail fast for unrecognized network names.
- Around line 79-114: The handler passed to
peerGroup.addPreMessageReceivedEventListener currently sets
mnlistdiffReceivedFuture early and manually closes the OutputStream; instead,
wrap the FileOutputStream in a try-with-resources and move
mnlistdiffReceivedFuture.set(true) to after all processing (writing,
diff.getMnList iteration and other post-processing) completes successfully so
the future is only completed on success; ensure exceptions still propagate (or
complete exceptionally) and do not close the future prematurely when handling
SimplifiedMasternodeListDiff in the lambda that casts to
SimplifiedMasternodeListDiff and uses stream, diff, countLegacy/countEnabled and
evoNodes.
---
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java`:
- Around line 288-313: The close() method must cancel the scheduled maintenance
and wait for the message executor to terminate before nulling collaborators;
update close() to mirror stop() behavior by cancelling the schedule (future
returned from schedule), ensuring any scheduled doMaintenance() is cancelled,
and invoking the same shutdown + awaitTermination logic used in stop() for
messageProcessingExecutor (instead of only shutdown()), then clear blockChain
and peerGroup; reference the existing stop(), close(), schedule (the
ScheduledFuture), doMaintenance(), and messageProcessingExecutor symbols so the
cleanup sequence is performed safely.
In `@core/src/main/java/org/bitcoinj/evolution/QuorumRotationState.java`:
- Around line 444-468: isSynced() races on the non-volatile field blockChain;
capture a local snapshot (e.g., BlockChain chain = this.blockChain) at the start
of the method and use that local variable for all subsequent calls
(chain.getBestChainHeight(), chain.isInitialHeaderSyncComplete(), and passing to
params.isDIP0024Active) and for the null checks so the method cannot NPE if
shutdown nulls the field concurrently; ensure you return the same results by
checking snapshot == null where appropriate before using its methods.
In `@core/src/main/java/org/bitcoinj/manager/DashSystem.java`:
- Around line 255-297: The close() method calls dualBlockChain.close() unguarded
which can NPE if dualBlockChain was never assigned (it’s only set in
setPeerGroupAndBlockChain() and setMasternodeListManager() currently creates a
local DualBlockChain). Fix by either assigning the field in
setMasternodeListManager() (change the local instantiation to set
this.dualBlockChain = ...) or by adding a null-check in close() before calling
dualBlockChain.close() (if (dualBlockChain != null) { dualBlockChain.close();
}), referencing the dualBlockChain field, setMasternodeListManager(),
setPeerGroupAndBlockChain(), and close().
In `@core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java`:
- Around line 129-137: Capture the current blockChain field into a local final
variable at the start of the method that performs the early null check (in class
ChainLocksHandler, the method that references blockChain between the early check
and the dereference) to prevent close() from nulling the field mid-execution;
i.e., create a local variable (e.g., final BlockChain localChain =
this.blockChain) immediately after method entry and use localChain for all
subsequent null checks and dereferences instead of referencing the field
directly, leaving the close() method unchanged.
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 122-149: The race is that preMessageReceivedEventListener can call
processInstantSendLock() which reads blockChain before the instance lock is held
while close() can null blockChain; to fix, start processInstantSendLock() by
acquiring the existing lock (the same lock used to protect instance state)
before any access to blockChain, check if blockChain is null and return if so,
and release the lock in a finally block; this ensures callbacks via
preMessageReceivedEventListener cannot observe partially-closed state even
though close() does not unregister that listener.
In `@core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java`:
- Around line 551-582: The rebuild can race with processTransaction and
resurrect a just-used key; to fix, ensure the precomputed usedPubKeyHashes is
validated while holding the same lock used for committing the rebuilt maps:
compute or recompute usedPubKeyHashes after acquiring unusedKeysLock (or,
alternatively, after acquiring unusedKeysLock re-scan
wallet.getTransactions(true) to add any new outputs) before mutating unusedKeys
and keyUsage; keep references to unusedKeysLock, wallet.getTransactions(true),
usedPubKeyHashes, unusedKeys, keyUsage, issuedKeys and processTransaction so
reviewers can find and verify the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3daa7559-5da2-4824-8fde-8e4964819b6b
📒 Files selected for processing (16)
core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.javacore/src/main/java/org/bitcoinj/core/AbstractManager.javacore/src/main/java/org/bitcoinj/core/DualBlockChain.javacore/src/main/java/org/bitcoinj/core/MasternodeSync.javacore/src/main/java/org/bitcoinj/core/PeerGroup.javacore/src/main/java/org/bitcoinj/core/SporkManager.javacore/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.javacore/src/main/java/org/bitcoinj/evolution/QuorumRotationState.javacore/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.javacore/src/main/java/org/bitcoinj/governance/GovernanceManager.javacore/src/main/java/org/bitcoinj/manager/DashSystem.javacore/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.javacore/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.javaexamples/src/main/java/org/bitcoinj/examples/DumpMasternodeList.java
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java (1)
894-903:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
close()should defensively unregister listeners before nulling references.Right now
close()nullspeerGroup/blockChaindirectly. If any caller path forgets to callremoveEventListeners(...)first, registered listeners may still fire and hitblockChain/state access after shutdown, causing intermittent NPEs.Suggested defensive fix
public void close() { // reset the state of any sync operation waitingForMNListDiff = false; + // defensively detach listeners before dropping references + removeEventListeners(blockChain != null ? blockChain.getBlockChain() : null, peerGroup); if (retryFuture != null) { log.info("cancel: {}", lastRequest.request.getClass().getSimpleName()); retryFuture.cancel(true); retryFuture = null; } peerGroup = null; blockChain = null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java` around lines 894 - 903, In close() you need to defensively unregister any registered listeners before nulling peerGroup and blockChain: call the existing removeEventListeners(...) (or directly unregister the specific listeners/handlers held by AbstractQuorumState) while peerGroup and blockChain are still valid, cancel retryFuture as before, then set peerGroup = null and blockChain = null; ensure removeEventListeners is invoked even if callers forgot to call it and that it safely handles nulls so no listeners run after shutdown.core/src/main/java/org/bitcoinj/core/MasternodeSync.java (1)
125-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent shutdown-time NPE after nulling shared references.
After Line 129–130, async callbacks can still call
updateBlockTip(...), which dereferencesblockChainat Line 560 without a null guard. That creates a shutdown race and potential NPE.Suggested fix
+ private final AtomicBoolean closed = new AtomicBoolean(false); + public void close() { + closed.set(true); if (peerGroup != null) { peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener); } peerGroup = null; blockChain = null; }public void updateBlockTip(StoredBlock storedBlock, boolean initialDownload) { + if (closed.get() || blockChain == null) { + return; + } if (!initialDownload && storedBlock.getHeight() % 100 == 0) log.info("updateBlockTip: height: {} initialDownload={}", storedBlock.getHeight(), initialDownload);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/org/bitcoinj/core/MasternodeSync.java` around lines 125 - 131, The shutdown sequence can race with async callbacks: ensure updateBlockTip(...) safely handles the object being closed by adding a null guard at its start (e.g., if blockChain == null return) and/or check a new volatile boolean closed flag set in close() before nulling fields; in close() keep the removePreMessageReceivedEventListener(preMessageReceivedEventListener) call, set closed = true (or similar) and only then null peerGroup and blockChain so updateBlockTip and other callbacks see the closed flag or the null-check and avoid dereferencing blockChain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java`:
- Around line 206-207: The null check on the field blockChain in
processNewChainLock is racy because blockChain is not declared volatile and
close() does not synchronize with the lock; fix by declaring the field
blockChain as volatile to ensure visibility, and additionally make close()
acquire the same lock (lock) before setting blockChain = null or re-check
blockChain inside the lock in processNewChainLock just before using
blockChain.getBlockStore().get(...); update either close() or
processNewChainLock (or both) so the write-to-null and reads are synchronized on
lock to prevent TOCTOU NPEs.
---
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/core/MasternodeSync.java`:
- Around line 125-131: The shutdown sequence can race with async callbacks:
ensure updateBlockTip(...) safely handles the object being closed by adding a
null guard at its start (e.g., if blockChain == null return) and/or check a new
volatile boolean closed flag set in close() before nulling fields; in close()
keep the removePreMessageReceivedEventListener(preMessageReceivedEventListener)
call, set closed = true (or similar) and only then null peerGroup and blockChain
so updateBlockTip and other callbacks see the closed flag or the null-check and
avoid dereferencing blockChain.
In `@core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java`:
- Around line 894-903: In close() you need to defensively unregister any
registered listeners before nulling peerGroup and blockChain: call the existing
removeEventListeners(...) (or directly unregister the specific
listeners/handlers held by AbstractQuorumState) while peerGroup and blockChain
are still valid, cancel retryFuture as before, then set peerGroup = null and
blockChain = null; ensure removeEventListeners is invoked even if callers forgot
to call it and that it safely handles nulls so no listeners run after shutdown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac12aefb-ab93-40d6-95e9-48d45f5bafd2
📒 Files selected for processing (10)
core/src/main/java/org/bitcoinj/core/DualBlockChain.javacore/src/main/java/org/bitcoinj/core/MasternodeSync.javacore/src/main/java/org/bitcoinj/core/SporkManager.javacore/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.javacore/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.javacore/src/main/java/org/bitcoinj/governance/GovernanceManager.javacore/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.javacore/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java
🚧 Files skipped from review as they are similar to previous changes (6)
- core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
- core/src/main/java/org/bitcoinj/governance/GovernanceManager.java
- core/src/main/java/org/bitcoinj/core/SporkManager.java
- core/src/main/java/org/bitcoinj/core/DualBlockChain.java
- core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java
- core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
|
@HashEngineering build bls library failed |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java (1)
293-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for in-flight DSQ tasks before clearing network state.
Removing the listeners stops new callbacks, but in-flight callbacks continue to execute.
shutdown()does not wait for queued or running work to complete—it only prevents new submissions. Theclose()method then immediately nullsblockChainandpeerGroup, while DSQUEUE tasks may still be running and accessing those fields, creating a race condition. Thestop()method in this same class demonstrates the correct pattern: callshutdown(), thenawaitTermination(10, TimeUnit.SECONDS)with fallback toshutdownNow()on timeout, and only then clear those fields.Apply the same termination logic to
close():Proposed fix
if (execToStop != null) { execToStop.shutdown(); + try { + if (!execToStop.awaitTermination(10, TimeUnit.SECONDS)) { + log.warn("CoinJoin message processing executor did not terminate in time, forcing shutdown"); + execToStop.shutdownNow(); + } + } catch (InterruptedException e) { + execToStop.shutdownNow(); + Thread.currentThread().interrupt(); + } } blockChain = null; peerGroup = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java` around lines 293 - 315, The close() implementation currently calls shutdown() on messageProcessingExecutor but immediately clears network state (blockChain, peerGroup) causing races with in-flight DSQ tasks; modify the logic that handles messageProcessingExecutor (the same area that sets execToStop) to mirror stop(): after execToStop.shutdown() call await termination (e.g., execToStop.awaitTermination(10, TimeUnit.SECONDS)) and if it times out call execToStop.shutdownNow(); only after successful termination (or after shutdownNow) clear blockChain and peerGroup; ensure you still use the existing lock/messageProcessingExecutor nulling pattern and reference the same symbols messageProcessingExecutor, execToStop, stop() behavior when implementing the wait-and-fallback termination.
♻️ Duplicate comments (1)
core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java (1)
206-222:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPartial fix: second locked section at line 271 remains vulnerable
The double-check at lines 207 and 211 reduces the race window, but two issues remain:
Visibility:
blockChain(line 67) is notvolatile. The null write inclose()(line 131) may not be visible to threads reading the field.Second locked section unguarded: Line 271 (
blockChain.getBlockStore().get(...)) executes in a separate locked section (lines 257–291) that has no null check. Sinceclose()never acquireslock, it can setblockChain = nullafter line 211 passes but before line 271 runs, causing an NPE.Complete the fix by declaring the field
volatileand adding a null check at line 258 (right after acquiring the second lock):🔒 Recommended complete fix
At line 67, declare the field
volatile:- AbstractBlockChain blockChain; + volatile AbstractBlockChain blockChain;At line 258 (start of second locked section), add a null check:
lock.lock(); try { + if (blockChain == null) return; // guard second locked section if (internalHasConflictingChainLock(clsig.height, clsig.blockHash)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java` around lines 206 - 222, Declare the blockChain field as volatile (the field referenced in close() and processNewChainLock()) to ensure visibility across threads, and in processNewChainLock() after acquiring lock for the second locked section (the block where blockChain.getBlockStore().get(...) is invoked) add an immediate null check like "if (blockChain == null) return;" so you don't dereference a cleared blockChain; keep existing seenChainLocks and bestChainLock checks unchanged and ensure close() continues to set blockChain = null without acquiring lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java`:
- Around line 293-315: The close() implementation currently calls shutdown() on
messageProcessingExecutor but immediately clears network state (blockChain,
peerGroup) causing races with in-flight DSQ tasks; modify the logic that handles
messageProcessingExecutor (the same area that sets execToStop) to mirror stop():
after execToStop.shutdown() call await termination (e.g.,
execToStop.awaitTermination(10, TimeUnit.SECONDS)) and if it times out call
execToStop.shutdownNow(); only after successful termination (or after
shutdownNow) clear blockChain and peerGroup; ensure you still use the existing
lock/messageProcessingExecutor nulling pattern and reference the same symbols
messageProcessingExecutor, execToStop, stop() behavior when implementing the
wait-and-fallback termination.
---
Duplicate comments:
In `@core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java`:
- Around line 206-222: Declare the blockChain field as volatile (the field
referenced in close() and processNewChainLock()) to ensure visibility across
threads, and in processNewChainLock() after acquiring lock for the second locked
section (the block where blockChain.getBlockStore().get(...) is invoked) add an
immediate null check like "if (blockChain == null) return;" so you don't
dereference a cleared blockChain; keep existing seenChainLocks and bestChainLock
checks unchanged and ensure close() continues to set blockChain = null without
acquiring lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a84fd15b-4d51-4d24-b2c4-5e29a59fb8d1
📒 Files selected for processing (4)
core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.javacore/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.javaexamples/src/main/java/org/bitcoinj/examples/DumpMasternodeList.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 17-21: Replace tag-pinned GitHub Actions with commit-SHA pinned
versions and disable checkout credential persistence: update the uses for
actions/checkout and actions/setup-java to reference their commit SHAs instead
of `@v4`, and add persist-credentials: false under the actions/checkout step (and
any checkout iterations) to prevent token leakage; ensure you update any related
inputs for actions/setup-java while keeping the same configuration but using the
SHA-pinned ref.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01a05583-b935-4cc0-8289-1e26a3a6041f
📒 Files selected for processing (1)
.github/workflows/tests.yml
abaranouski
approved these changes
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Documentation
Tests
Chores