Fix/refresh unused keys#296
Open
HashEngineering wants to merge 6 commits intomasterfrom
Open
Conversation
* don't remove peer listeners, that will be handled by PeerGroup
📝 WalkthroughWalkthroughThis pull request refactors listener lifecycle management across multiple manager classes to avoid deadlocks during shutdown. Rather than explicitly unregistering peer-group event listeners in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+134
to
+155
| public void setPeerGroup(PeerGroup peerGroup, MasternodeSync masternodeSync) { | ||
| if (peerGroup != null) { | ||
| this.peerGroup = peerGroup; | ||
| downloadProgressTracker = new MyDownloadProgressTracker(masternodeSync.hasSyncFlag(MasternodeSync.SYNC_FLAGS.SYNC_BLOCKS_AFTER_PREPROCESSING)); | ||
| peerGroup.addHeadersDownloadedEventListener(Threading.USER_THREAD, downloadProgressTracker); | ||
| peerGroup.addHeadersDownloadStartedEventListener(Threading.USER_THREAD, downloadProgressTracker); | ||
| peerGroup.addBlocksDownloadedEventListener(Threading.USER_THREAD, downloadProgressTracker); | ||
| peerGroup.addChainDownloadStartedEventListener(Threading.USER_THREAD, downloadProgressTracker); | ||
| peerGroup.addMasternodeListDownloadListener(Threading.USER_THREAD, downloadProgressTracker); | ||
| } | ||
| } | ||
|
|
||
| public boolean isInitialHeaderSyncComplete() { | ||
| return downloadProgressTracker != null && (downloadProgressTracker.headerDownloadCompleted || downloadProgressTracker.blockDownloadCompleted); | ||
| } | ||
|
|
||
| public void close() { | ||
| if (peerGroup != null) { | ||
| // no need to check remove event listeners from peergroup | ||
| peerGroup = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
close() is leaving a PeerGroup-wide tracker registered.
The listeners added in Lines 138-142 are attached to PeerGroup, not to individual peers, so the per-peer shutdown reasoning used elsewhere does not apply here. close() only nulls peerGroup, which leaves downloadProgressTracker registered on the old group and able to keep receiving callbacks after shutdown; calling setPeerGroup() again would also register an additional tracker. This needs explicit detach/replace handling around the stored tracker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/bitcoinj/core/DualBlockChain.java` around lines 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.
Comment on lines
+107
to
+116
| public void close() { | ||
| // No per-peer listener removal needed here: | ||
| // - preMessageReceivedEventListener is removed from each peer by PeerGroup.handlePeerDeath() | ||
| // - peerConnectedEventListener left on a disconnecting peer is harmless (it will never fire) | ||
| // Calling removeConnectedEventListener/removePreMessageReceivedEventListener would iterate | ||
| // getConnectedPeers() under the PeerGroup lock, which can deadlock during shutdown when | ||
| // another PeerGroup thread holds that lock while blocked on SPVBlockStore I/O. | ||
| peerGroup = null; | ||
| blockChain = null; | ||
| masternodeSync = null; |
There was a problem hiding this comment.
Detach the PeerGroup-level connect listener before dropping the reference.
Skipping preMessageReceivedEventListener cleanup makes sense because that one is removed per peer, but peerConnectedEventListener is registered on the PeerGroup itself in Lines 96-98. close() now nulls peerGroup without unregistering it, so this SporkManager can stay reachable and still send GetSporksMessage on later connections; a subsequent setBlockChain() call would also stack duplicate 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/core/SporkManager.java` around lines 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.
Comment on lines
+1718
to
+1721
| // All peerGroup.remove*EventListener calls skipped: handlePeerDeath() removes per-peer listeners | ||
| // when peers disconnect. Calling these methods acquires the PeerGroup lock via getConnectedPeers(), | ||
| // risking shutdown deadlock. | ||
| peerGroup = null; |
There was a problem hiding this comment.
close() drops the reference but does not detach registered listeners.
Lines 1718-1721 only null the local peerGroup. The PeerGroup listener registries still hold getDataEventListener / preMessageReceivedEventListener, so this manager can remain referenced and continue receiving callbacks if peers are still active.
💡 Proposed fix
`@Override`
public void close() {
- // All peerGroup.remove*EventListener calls skipped: handlePeerDeath() removes per-peer listeners
- // when peers disconnect. Calling these methods acquires the PeerGroup lock via getConnectedPeers(),
- // risking shutdown deadlock.
- peerGroup = null;
+ PeerGroup pg = peerGroup;
+ peerGroup = null;
+ if (pg != null && !pg.isRunning()) {
+ pg.removeGetDataEventListener(getDataEventListener);
+ pg.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/bitcoinj/governance/GovernanceManager.java` around
lines 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.
Comment on lines
+79
to
+114
| peerGroup.addPreMessageReceivedEventListener(SAME_THREAD, (peer1, m) -> { | ||
| try { | ||
| if (m instanceof SimplifiedMasternodeListDiff) { | ||
| System.out.println("Received mnlistdiff..."); | ||
| File dumpFile = new File(params.getNetworkName() + "-mnlist.dat"); | ||
| OutputStream stream = new FileOutputStream(dumpFile); | ||
| stream.write(m.bitcoinSerialize()); | ||
| stream.close(); | ||
| mnlistdiffReceivedFuture.set(true); | ||
| SimplifiedMasternodeListDiff diff = (SimplifiedMasternodeListDiff)m; | ||
| AtomicInteger countLegacy = new AtomicInteger(); | ||
| AtomicInteger countEnabled = new AtomicInteger(); | ||
| ArrayList<Masternode> evoNodes = new ArrayList<>(); | ||
| diff.getMnList().forEach(entry -> { | ||
| if (entry.isValid()) { | ||
| countLegacy.addAndGet((short) (entry.getVersion() == 1 ? 1 : 0)); | ||
| countEnabled.addAndGet(1); | ||
| if (MasternodeType.getMasternodeType(entry.getType()) == MasternodeType.HIGHPERFORMANCE) { | ||
| evoNodes.add(entry); | ||
| } | ||
| //System.out.println(countEnabled + " " + entry.getService().getAddr().getHostAddress()); | ||
| } | ||
| }); | ||
| System.out.printf("Total: %d, Legacy: %d\n", countEnabled.get(), countLegacy.get()); | ||
| System.out.println("HP Masternode List"); | ||
| evoNodes.forEach(entry -> System.out.println("\"" + entry.getService().getAddr().getHostAddress() + "\",")); | ||
| return null; | ||
| } | ||
| } catch (FileNotFoundException e) { | ||
| System.out.println("cannot find the file to write to"); | ||
| throw new RuntimeException(e); | ||
| } catch (IOException e) { | ||
| System.out.println("IO Error"); | ||
| e.printStackTrace(); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Don't signal success before the dump is fully processed.
mnlistdiffReceivedFuture is completed before the post-processing finishes, and the file stream is closed manually. If anything fails on this path, main() can block forever on get() or proceed as if the dump succeeded. Use try-with-resources and only complete the future after all processing has succeeded.
🛠 Suggested fix
if (m instanceof SimplifiedMasternodeListDiff) {
System.out.println("Received mnlistdiff...");
File dumpFile = new File(params.getNetworkName() + "-mnlist.dat");
- OutputStream stream = new FileOutputStream(dumpFile);
- stream.write(m.bitcoinSerialize());
- stream.close();
- mnlistdiffReceivedFuture.set(true);
- SimplifiedMasternodeListDiff diff = (SimplifiedMasternodeListDiff)m;
+ try (OutputStream stream = new FileOutputStream(dumpFile)) {
+ stream.write(m.bitcoinSerialize());
+ }
+ SimplifiedMasternodeListDiff diff = (SimplifiedMasternodeListDiff) m;
AtomicInteger countLegacy = new AtomicInteger();
AtomicInteger countEnabled = new AtomicInteger();
ArrayList<Masternode> evoNodes = new ArrayList<>();
diff.getMnList().forEach(entry -> {
if (entry.isValid()) {
@@
});
System.out.printf("Total: %d, Legacy: %d\n", countEnabled.get(), countLegacy.get());
System.out.println("HP Masternode List");
evoNodes.forEach(entry -> System.out.println("\"" + entry.getService().getAddr().getHostAddress() + "\","));
+ mnlistdiffReceivedFuture.set(true);
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/src/main/java/org/bitcoinj/examples/DumpMasternodeList.java` around
lines 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.
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
1 participant
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
Chores