Skip to content

Fix/refresh unused keys #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
HashEngineering wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/refresh unused keys #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c02a7b
fix: improve refreshUnusedKeys for locks
HashEngineering Apr 29, 2026
5cccfde
fix: improve getMixingProgress algorithm accuracy
HashEngineering Apr 29, 2026
7a8fc0e
fix: add test for improved getMixingProgress
HashEngineering Apr 29, 2026
b5d24f7
fix: PeerGroup: only start downloading from new peer if running
HashEngineering Apr 29, 2026
38be8af
fix: improve shutdown/close operations
HashEngineering Apr 29, 2026
28992b7
feat (examples): Add DumpMasternodeList
HashEngineering Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,13 +290,11 @@ public void close() {
blockChain.removeNewBestBlockListener(newBestBlockListener);
blockChain.removeTransactionReceivedListener(transactionReceivedInBlockListener);
}
if (peerGroup != null) {
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
if (masternodeGroup != null) {
masternodeGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
// Ensure executor is shut down
// removePreMessageReceivedEventListener skipped on both peerGroup and masternodeGroup:
// handlePeerDeath() removes it per-peer on disconnect. Calling it here acquires the
// PeerGroup lock via getConnectedPeers(), risking shutdown deadlock.

// Shut down the executor before nulling fields — queued tasks may still reference them.
ExecutorService execToStop = null;
lock.lock();
try {
Expand All @@ -310,6 +308,8 @@ public void close() {
if (execToStop != null) {
execToStop.shutdown();
}
blockChain = null;
peerGroup = null;
}

public boolean isMasternodeOrDisconnectRequested(MasternodeAddress address) {
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/bitcoinj/core/AbstractManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,9 @@ public void setFilename(String filename) {
autosaveToFile(new File(filename), DELAY_TIME, TimeUnit.MILLISECONDS, null);
}

/**
* Typically called when DashSystem is shutting down.
*/
public void close() {
if (vFileManager != null) {
shutdownAutosaveAndWait();
Expand Down
48 changes: 48 additions & 0 deletions core/src/main/java/org/bitcoinj/core/DualBlockChain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@

package org.bitcoinj.core;

import org.bitcoinj.core.listeners.DownloadProgressTracker;
import org.bitcoinj.store.BlockStoreException;
import org.bitcoinj.utils.Threading;

import javax.annotation.Nullable;

Expand Down Expand Up @@ -105,4 +107,50 @@ public StoredBlock getBlock(int height) {
public StoredBlock getChainHead() {
return getLongestChain().chainHead;
}

PeerGroup peerGroup;
MyDownloadProgressTracker downloadProgressTracker;

private static class MyDownloadProgressTracker extends DownloadProgressTracker {
volatile boolean headerDownloadCompleted = false;
volatile boolean blockDownloadCompleted = false;
MyDownloadProgressTracker(boolean preprocessingBeforeBlocks) {
super(preprocessingBeforeBlocks);
}

@Override
public void doneHeaderDownload() {
super.doneHeaderDownload();
headerDownloadCompleted = true;
}

@Override
protected void doneDownload() {
super.doneDownload();
blockDownloadCompleted = true;
}
}

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;
}
}
Comment on lines +134 to +155
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
8 changes: 5 additions & 3 deletions core/src/main/java/org/bitcoinj/core/MasternodeSync.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,11 @@ public void setBlockChain(AbstractBlockChain blockChain, PeerGroup peerGroup, Ne
}

public void close() {
if (peerGroup != null) {
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
// No per-peer listener removal needed: handlePeerDeath() cleans up preMessageReceivedEventListener
// from each peer when it disconnects. Calling removePreMessageReceivedEventListener() here would
// acquire the PeerGroup lock via getConnectedPeers(), which can deadlock during shutdown.
peerGroup = null;
blockChain = null;
}

public MasternodeSync(Context context, boolean isLiteMode, boolean allowInstantSendInLiteMode) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/bitcoinj/core/PeerGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2044,7 +2044,7 @@ protected void handlePeerDeath(final Peer peer, @Nullable Throwable exception) {
final Peer newDownloadPeer = selectDownloadPeer(peers);
if (newDownloadPeer != null) {
setDownloadPeer(newDownloadPeer);
if (downloadListener != null) {
if (downloadListener != null && vRunning) {
startBlockChainDownloadFromPeer(newDownloadPeer);
}
}
Expand Down
19 changes: 14 additions & 5 deletions core/src/main/java/org/bitcoinj/core/SporkManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ private static void makeSporkDefinition(SporkId sporkId, long defaultValue) {
@GuardedBy("lock") private final HashMap<SporkId, Long> mapSporksCachedValues;
@GuardedBy("lock") private final HashSet<KeyId> setSporkPubKeyIds = new HashSet<>();

private PeerGroup peerGroup;
private AbstractBlockChain blockChain;
private MasternodeSync masternodeSync;
private final Context context;
Expand All @@ -91,6 +92,7 @@ public SporkManager(Context context)
public void setBlockChain(AbstractBlockChain blockChain, @Nullable PeerGroup peerGroup, MasternodeSync masternodeSync) {
this.blockChain = blockChain;
this.masternodeSync = masternodeSync;
this.peerGroup = peerGroup;
if (peerGroup != null) {
peerGroup.addConnectedEventListener(peerConnectedEventListener);
peerGroup.addPreMessageReceivedEventListener(SAME_THREAD, preMessageReceivedEventListener);
Expand All @@ -102,18 +104,25 @@ public void clear() {
mapSporksByHash.clear();
}

public void close(PeerGroup peerGroup) {
if (peerGroup != null) {
peerGroup.removeConnectedEventListener(peerConnectedEventListener);
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
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;
Comment on lines +107 to +116
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}

void processSpork(Peer from, SporkMessage spork) {
// if (context.isLiteMode() && !context.allowInstantXinLiteMode()) {
// return; //disable all darksend/masternode related functionality
// }

if (blockChain == null) return; // closed during shutdown

Sha256Hash hash = spork.getHash();
String logMessage = String.format("SPORK -- hash: %s id: %d (%s) value: %10d bestHeight: %d peer=%s:%d",
hash, spork.getSporkId().value, String.format("%1$35s", spork.getSporkId().name()),
Expand Down
14 changes: 8 additions & 6 deletions core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,12 +554,10 @@ public void removeEventListeners(AbstractBlockChain blockChain, PeerGroup peerGr
blockChain.removeNewBestBlockListener(newBestBlockListener);
blockChain.removeReorganizeListener(reorganizeListener);
}
if (peerGroup != null) {
peerGroup.removeConnectedEventListener(peerConnectedEventListener);
peerGroup.removeChainDownloadStartedEventListener(chainDownloadStartedEventListener);
peerGroup.removeHeadersDownloadStartedEventListener(headersDownloadStartedEventListener);
peerGroup.removeDisconnectedEventListener(peerDisconnectedEventListener);
}
// All peerGroup.remove*EventListener calls skipped: handlePeerDeath() removes per-peer listeners
// (ChainDownloadStarted, Disconnected) when peers disconnect, and Connected/HeadersDownloadStarted
// listeners left on dying peers are benign (those events never fire on disconnecting peers).
// Calling these methods acquires the PeerGroup lock via getConnectedPeers(), risking shutdown deadlock.
}

public final NewBestBlockListener newBestBlockListener = new NewBestBlockListener() {
Expand Down Expand Up @@ -597,6 +595,7 @@ public void notifyNewBestBlock(StoredBlock block) throws VerificationException {
public final PeerConnectedEventListener peerConnectedEventListener = new PeerConnectedEventListener() {
@Override
public void onPeerConnected(Peer peer, int peerCount) {
if (peerGroup == null) return; // closed during shutdown
downloadPeer = peerGroup.getDownloadPeer();
log.info("peer connected and setting download peer to {} with onPeerConnected", downloadPeer);
}
Expand All @@ -605,6 +604,7 @@ public void onPeerConnected(Peer peer, int peerCount) {
final PeerDisconnectedEventListener peerDisconnectedEventListener = new PeerDisconnectedEventListener() {
@Override
public void onPeerDisconnected(Peer peer, int peerCount) {
if (peerGroup == null) return; // closed during shutdown
if (downloadPeer == peer) {
downloadPeer = peerGroup.getDownloadPeer();
log.info("setting download peer to {} with onPeerDisconnected, previously was {}", downloadPeer, peer);
Expand Down Expand Up @@ -897,5 +897,7 @@ public void close() {
retryFuture.cancel(true);
retryFuture = null;
}
peerGroup = null;
blockChain = null;
}
}
13 changes: 10 additions & 3 deletions core/src/main/java/org/bitcoinj/evolution/QuorumRotationState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,13 +445,20 @@ public boolean isSynced() {
if (blockChain != null && !params.isDIP0024Active(blockChain.getBestChainHeight()))
return true;

if(mnListAtH.getHeight() == -1)
if (mnListAtH.getHeight() == -1)
return false;

if (peerGroup == null)
if (blockChain == null)
return false;

int mostCommonHeight = peerGroup.getMostCommonHeight();
if (!blockChain.isInitialHeaderSyncComplete()) {
return false;
}

// Use local chain height instead of peerGroup.getMostCommonHeight() to avoid
// acquiring the PeerGroup lock, which can deadlock during shutdown or when the
// PeerGroup thread holds it while blocked on SPVBlockStore I/O.
int mostCommonHeight = blockChain.getBestChainHeight();

// determine when the last QR height was
LLMQParameters llmqParameters = params.getLlmqs().get(llmqType);
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,15 +443,16 @@ public void close() {
quorumState.close();
quorumRotationState.close();

if (peerGroup != null) {
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
// removePreMessageReceivedEventListener skipped: handlePeerDeath() removes it per-peer on disconnect.
// Calling it here acquires the PeerGroup lock via getConnectedPeers(), risking shutdown deadlock.
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
}
peerGroup = null;
blockChain = null;
saveNow(); // Always save, regardless of thread pool state
super.close();
Comment thread
HashEngineering marked this conversation as resolved.

Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/org/bitcoinj/governance/GovernanceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1715,9 +1715,9 @@ public void run() {

@Override
public void close() {
if (peerGroup != null) {
peerGroup.removeGetDataEventListener(getDataEventListener);
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
// 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;
Comment on lines +1718 to +1721
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}
}
11 changes: 9 additions & 2 deletions core/src/main/java/org/bitcoinj/manager/DashSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public class DashSystem {
public AbstractBlockChain blockChain;
@Nullable
public AbstractBlockChain headerChain;
DualBlockChain dualBlockChain;
public SporkManager sporkManager;
public MasternodePayments masternodePayments;
public MasternodeSync masternodeSync;
Expand Down Expand Up @@ -251,10 +252,13 @@ private void stopLLMQThread() {
}
}

/**
* close down DashSystem and free resources, typically called after the PeerGroup is shutdown
*/
public void close() {
if (initializedObjects) {
log.info("Closing network spork configuration manager");
sporkManager.close(peerGroup);
sporkManager.close();
log.info("Closing masternode synchronization state");
masternodeSync.close();
log.info("Closing masternode list manager (includes thread pool shutdown)");
Expand Down Expand Up @@ -285,6 +289,8 @@ public void close() {
log.info("Closing header chain");
headerChain.close();
}
log.info("closing dual blockchain");
dualBlockChain.close();
log.info("Clearing peer group reference");
peerGroup = null;
}
Expand All @@ -295,13 +301,14 @@ public void setPeerGroupAndBlockChain(PeerGroup peerGroup, AbstractBlockChain bl
this.peerGroup = peerGroup;
this.blockChain = blockChain;
this.headerChain = headerChain;
DualBlockChain dualBlockChain = new DualBlockChain(headerChain, blockChain);
dualBlockChain = new DualBlockChain(headerChain, blockChain);
hashStore = new HashStore(blockChain.getBlockStore());
blockChain.addNewBestBlockListener(newBestBlockListener);
handleActivations(blockChain.getChainHead());
if (initializedObjects) {
sporkManager.setBlockChain(blockChain, peerGroup, masternodeSync);
masternodeSync.setBlockChain(blockChain, peerGroup, netFullfilledRequestManager, governanceManager);
dualBlockChain.setPeerGroup(peerGroup, masternodeSync);
masternodeListManager.setBlockChain(
dualBlockChain,
peerGroup,
Expand Down
10 changes: 5 additions & 5 deletions core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,8 @@ public void setBlockChain(PeerGroup peerGroup, AbstractBlockChain blockChain, Ab
public void close() {
blockChain = null;
headerChain = null;
if (peerGroup != null) {
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
}
// removePreMessageReceivedEventListener skipped: handlePeerDeath() removes it per-peer on disconnect.
// Calling it here acquires the PeerGroup lock via getConnectedPeers(), risking shutdown deadlock.
peerGroup = null;
super.close();
}
Expand Down Expand Up @@ -203,8 +202,9 @@ public void processChainLockSignature(Peer peer, ChainLockSignature clsig)
processNewChainLock(peer, clsig, hash);
}

void processNewChainLock(Peer from, ChainLockSignature clsig, Sha256Hash hash)
{
void processNewChainLock(Peer from, ChainLockSignature clsig, Sha256Hash hash) {
if (blockChain == null) return; // closed during shutdown

lock.lock();
try {
if (seenChainLocks.put(hash, Utils.currentTimeMillis()) != null) {
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,15 @@ public void close(PeerGroup peerGroup) {
}
if (peerGroup != null) {
peerGroup.removeOnTransactionBroadcastListener(this.transactionBroadcastListener);
peerGroup.removePreMessageReceivedEventListener(preMessageReceivedEventListener);
// removePreMessageReceivedEventListener skipped: handlePeerDeath() removes it per-peer on disconnect.
// Calling it here acquires the PeerGroup lock via getConnectedPeers(), risking shutdown deadlock.
}
if (chainLocksHandler != null) {
chainLocksHandler.removeChainLockListener(this.chainLockListener);
}
blockChain = null;
peerGroup = null;
chainLocksHandler = null;
wallets.forEach(wallet -> wallet.removeCoinsSentEventListener(coinsSentEventListener));
try {
if (!scheduledExecutorService.awaitTermination(3000, TimeUnit.MILLISECONDS)) {
Expand Down
Loading
Loading