Move cycle detecting lock creation out into a new Locks class, rather than stuff it into Utils.

Convert PeerGroup and Peer to also use cycle detecting locks, and add a unit test to Wallet to check that cycle detection works.
 Change default policy to warn. Now warnings are being triggered, the followup commits will fix them.

core/src/main/java/com/google/bitcoin/core/Peer.java

@@ -19,6 +19,7 @@ package com.google.bitcoin.core;
 import com.google.bitcoin.store.BlockStore;
 import com.google.bitcoin.store.BlockStoreException;
 import com.google.bitcoin.utils.EventListenerInvoker;
+import com.google.bitcoin.utils.Locks;
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
@@ -38,6 +39,8 @@ import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.ReentrantLock;
 
+import static com.google.common.base.Preconditions.checkState;
+
 /**
  * A Peer handles the high level communication with a Bitcoin node.
  *
@@ -52,6 +55,7 @@ public class Peer {
     }
 
     private static final Logger log = LoggerFactory.getLogger(Peer.class);
+    protected final ReentrantLock lock = Locks.lock("peer");
 
     private final NetworkParameters params;
     private final AbstractBlockChain blockChain;
@@ -171,7 +175,9 @@ public class Peer {
     }
 
     @Override
-    public synchronized String toString() {
+    public String toString() {
+        lock.lock();
+        try {
             PeerAddress addr = address.get();
             if (addr == null) {
                 // User-provided NetworkConnection object.
@@ -179,6 +185,9 @@ public class Peer {
             } else {
                 return "Peer(" + addr.getAddr() + ":" + addr.getPort() + ")";
             }
+        } finally {
+            lock.unlock();
+        }
     }
 
     private void notifyDisconnect() {
@@ -226,22 +235,22 @@ public class Peer {
 
             // Allow event listeners to filter the message stream. Listeners are allowed to drop messages by
             // returning null.
-            synchronized (Peer.this) {
+            lock.lock();
+            try {
                 for (PeerEventListener listener : eventListeners) {
                     synchronized (listener) {
                         m = listener.onPreMessageReceived(Peer.this, m);
                         if (m == null) break;
                     }
                 }
-            }
-
                 if (m == null) return;
 
-            synchronized (Peer.this) {
                 if (currentFilteredBlock != null && !(m instanceof Transaction)) {
                     processFilteredBlock(currentFilteredBlock);
                     currentFilteredBlock = null;
                 }
+            } finally {
+                lock.unlock();
             }
 
             if (m instanceof NotFoundMessage) {
@@ -257,8 +266,11 @@ public class Peer {
                 // messages stream in. We'll call processFilteredBlock when a non-tx message arrives (eg, another
                 // FilteredBlock) or when a tx that isn't needed by that block is found. A ping message is sent after
                 // a getblocks, to force the non-tx message path.
-                synchronized (Peer.this) {
+                lock.lock();
+                try {
                     currentFilteredBlock = (FilteredBlock) m;
+                } finally {
+                    lock.unlock();
                 }
             } else if (m instanceof Transaction) {
                 processTransaction((Transaction) m);
@@ -309,6 +321,8 @@ public class Peer {
     }
 
     private void processNotFoundMessage(NotFoundMessage m) {
+        // This does not need to be locked.
+
         // This is received when we previously did a getdata but the peer couldn't find what we requested in it's
         // memory pool. Typically, because we are downloading dependencies of a relevant transaction and reached
         // the bottom of the dependency tree (where the unconfirmed transactions connect to transactions that are
@@ -327,7 +341,8 @@ public class Peer {
         }
     }
 
-    private synchronized void processAlert(AlertMessage m) {
+    private void processAlert(AlertMessage m) {
+        // This does not need to be locked.
         try {
             if (m.isSignatureValid()) {
                 log.info("Received alert from peer {}: {}", toString(), m.getStatusBar());
@@ -347,16 +362,16 @@ public class Peer {
         return handler;
     }
 
-    private synchronized void processHeaders(HeadersMessage m) throws IOException, ProtocolException {
+    private void processHeaders(HeadersMessage m) throws IOException, ProtocolException {
         // Runs in network loop thread for this peer.
         //
         // This method can run if a peer just randomly sends us a "headers" message (should never happen), or more
         // likely when we've requested them as part of chain download using fast catchup. We need to add each block to
         // the chain if it pre-dates the fast catchup time. If we go past it, we can stop processing the headers and
         // request the full blocks from that point on instead.
-        Preconditions.checkState(!downloadBlockBodies, toString());
+        lock.lock();
-
         try {
+            checkState(!downloadBlockBodies, toString());
             for (int i = 0; i < m.getBlockHeaders().size(); i++) {
                 Block header = m.getBlockHeaders().get(i);
                 if (header.getTimeSeconds() < fastCatchupTimeSecs) {
@@ -392,10 +407,13 @@ public class Peer {
         } catch (PrunedException e) {
             // Unreachable when in SPV mode.
             throw new RuntimeException(e);
+        } finally {
+            lock.unlock();
         }
     }
 
-    private synchronized void processGetData(GetDataMessage getdata) throws IOException {
+    private void processGetData(GetDataMessage getdata) throws IOException {
+        // This does not need to be locked.
         log.info("{}: Received getdata message: {}", address.get(), getdata.toString());
         ArrayList<Message> items = new ArrayList<Message>();
         for (PeerEventListener listener : eventListeners) {
@@ -414,7 +432,9 @@ public class Peer {
         }
     }
 
-    private synchronized void processTransaction(Transaction tx) throws VerificationException, IOException {
+    private void processTransaction(Transaction tx) throws VerificationException, IOException {
+        lock.lock();
+        try {
             log.debug("{}: Received tx {}", address.get(), tx.getHashAsString());
             if (memoryPool != null) {
                 // We may get back a different transaction object.
@@ -485,6 +505,9 @@ public class Peer {
                     listener.onTransaction(Peer.this, fTx);
                 }
             });
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -541,7 +564,6 @@ public class Peer {
         for (TransactionInput input : tx.getInputs()) {
             // There may be multiple inputs that connect to the same transaction.
             Sha256Hash hash = input.getOutpoint().getHash();
-            synchronized (this) {
             Transaction dep = memoryPool.get(hash);
             if (dep == null) {
                 needToRequest.add(hash);
@@ -549,8 +571,8 @@ public class Peer {
                 dependencies.add(dep);
             }
         }
-        }
         results.addAll(dependencies);
+        lock.lock();
         try {
             // Build the request for the missing dependencies.
             List<ListenableFuture<Transaction>> futures = Lists.newArrayList();
@@ -631,13 +653,16 @@ public class Peer {
             log.error("Couldn't send getdata in downloadDependencies({})", tx.getHash());
             resultFuture.setException(e);
             return resultFuture;
+        } finally {
+            lock.unlock();
         }
         return resultFuture;
     }
 
-    private synchronized void processBlock(Block m) throws IOException {
+    private void processBlock(Block m) throws IOException {
         if (log.isDebugEnabled())
             log.debug("{}: Received broadcast block {}", address.get(), m.getHashAsString());
+        lock.lock();
         try {
             // Was this block requested by getBlock()?
             if (maybeHandleRequestedData(m)) return;
@@ -683,13 +708,16 @@ public class Peer {
         } catch (PrunedException e) {
             // Unreachable when in SPV mode.
             throw new RuntimeException(e);
+        } finally {
+            lock.unlock();
         }
     }
 
     // TODO: Fix this duplication.
-    private synchronized void processFilteredBlock(FilteredBlock m) throws IOException {
+    private void processFilteredBlock(FilteredBlock m) throws IOException {
         if (log.isDebugEnabled())
             log.debug("{}: Received broadcast filtered block {}", address.get(), m.getHash().toString());
+        lock.lock();
         try {
             if (!downloadData.get()) {
                 log.debug("{}: Received block we did not ask for: {}", address.get(), m.getHash().toString());
@@ -731,10 +759,13 @@ public class Peer {
             // data from the remote peer and fix things. Or just give up.
             // TODO: Request e.getHash() and submit it to the block store before any other blocks
             throw new RuntimeException(e);
+        } finally {
+            lock.unlock();
         }
     }
 
     private boolean maybeHandleRequestedData(Message m) {
+        checkState(lock.isLocked());
         boolean found = false;
         Sha256Hash hash = m.getHash();
         for (ListIterator<GetDataRequest> it = getDataFutures.listIterator(); it.hasNext();) {
@@ -749,7 +780,8 @@ public class Peer {
         return found;
     }
 
-    private synchronized void invokeOnBlocksDownloaded(final Block m) {
+    private void invokeOnBlocksDownloaded(final Block m) {
+        checkState(lock.isLocked());
         // It is possible for the peer block height difference to be negative when blocks have been solved and broadcast
         // since the time we first connected to the peer. However, it's weird and unexpected to receive a callback
         // with negative "blocks left" in this case, so we clamp to zero so the API user doesn't have to think about it.
@@ -762,7 +794,9 @@ public class Peer {
         });
     }
 
-    private synchronized void processInv(InventoryMessage inv) throws IOException {
+    private void processInv(InventoryMessage inv) throws IOException {
+        lock.lock();
+        try {
             // This should be called in the network loop thread for this peer.
             List<InventoryItem> items = inv.getItems();
 
@@ -772,9 +806,14 @@ public class Peer {
 
             for (InventoryItem item : items) {
                 switch (item.type) {
-                case Transaction: transactions.add(item); break;
+                    case Transaction:
-                case Block: blocks.add(item); break;
+                        transactions.add(item);
-                default: throw new IllegalStateException("Not implemented: " + item.type);
+                        break;
+                    case Block:
+                        blocks.add(item);
+                        break;
+                    default:
+                        throw new IllegalStateException("Not implemented: " + item.type);
                 }
             }
 
@@ -873,6 +912,9 @@ public class Peer {
 
             if (pingAfterGetData)
                 sendMessage(new Ping((long) Math.random() * Long.MAX_VALUE));
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -881,6 +923,7 @@ public class Peer {
      * will block until the peer answers.
      */
     public ListenableFuture<Block> getBlock(Sha256Hash blockHash) throws IOException {
+        // This does not need to be locked.
         log.info("Request to fetch block {}", blockHash);
         GetDataMessage getdata = new GetDataMessage(params);
         getdata.addBlock(blockHash);
@@ -893,6 +936,7 @@ public class Peer {
      * in future many peers will delete old transaction data they don't need.
      */
     public ListenableFuture<Transaction> getPeerMempoolTransaction(Sha256Hash hash) throws IOException {
+        // This does not need to be locked.
         // TODO: Unit test this method.
         log.info("Request to fetch peer mempool tx  {}", hash);
         GetDataMessage getdata = new GetDataMessage(params);
@@ -902,6 +946,7 @@ public class Peer {
 
     /** Sends a getdata with a single item in it. */
     private ListenableFuture sendSingleGetData(GetDataMessage getdata) throws IOException {
+        // This does not need to be locked.
         Preconditions.checkArgument(getdata.getItems().size() == 1);
         GetDataRequest req = new GetDataRequest();
         req.future = SettableFuture.create();
@@ -920,7 +965,9 @@ public class Peer {
      *
      * @param secondsSinceEpoch Time in seconds since the epoch or 0 to reset to always downloading block bodies.
      */
-    public synchronized void setDownloadParameters(long secondsSinceEpoch, boolean useFilteredBlocks) {
+    public void setDownloadParameters(long secondsSinceEpoch, boolean useFilteredBlocks) {
+        lock.lock();
+        try {
             Preconditions.checkNotNull(blockChain);
             if (secondsSinceEpoch == 0) {
                 fastCatchupTimeSecs = params.genesisBlock.getTimeSeconds();
@@ -934,6 +981,9 @@ public class Peer {
                 }
             }
             this.useFilteredBlocks = useFilteredBlocks;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -942,11 +992,13 @@ public class Peer {
      * independently, otherwise the wallet will receive duplicate notifications.
      */
     public void addWallet(Wallet wallet) {
+        // This does not need to be locked.
         wallets.add(wallet);
     }
 
     /** Unlinks the given wallet from peer. See {@link Peer#addWallet(Wallet)}. */
     public void removeWallet(Wallet wallet) {
+        // This does not need to be locked.
         wallets.remove(wallet);
     }
 
@@ -954,6 +1006,7 @@ public class Peer {
      * Sends the given message on the peers Channel.
      */
     public ChannelFuture sendMessage(Message m) throws IOException {
+        // This does not need to be locked.
         return Channels.write(channel, m);
     }
 
@@ -962,7 +1015,7 @@ public class Peer {
     // multiple threads simultaneously.
     private Sha256Hash lastGetBlocksBegin, lastGetBlocksEnd;
 
-    private synchronized void blockChainDownload(Sha256Hash toHash) throws IOException {
+    private void blockChainDownload(Sha256Hash toHash) throws IOException {
         // The block chain download process is a bit complicated. Basically, we start with one or more blocks in a
         // chain that we have from a previous session. We want to catch up to the head of the chain BUT we don't know
         // where that chain is up to or even if the top block we have is even still in the chain - we
@@ -996,6 +1049,8 @@ public class Peer {
         // headers and then request the blocks from that point onwards. "getheaders" does not send us an inv, it just
         // sends us the data we requested in a "headers" message.
 
+        lock.lock();
+        try {
             // TODO: Block locators should be abstracted out rather than special cased here.
             List<Sha256Hash> blockLocator = new ArrayList<Sha256Hash>(51);
             // For now we don't do the exponential thinning as suggested here:
@@ -1043,21 +1098,26 @@ public class Peer {
                 GetHeadersMessage message = new GetHeadersMessage(params, blockLocator, toHash);
                 sendMessage(message);
             }
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
      * Starts an asynchronous download of the block chain. The chain download is deemed to be complete once we've
      * downloaded the same number of blocks that the peer advertised having in its version handshake message.
      */
-    public synchronized void startBlockChainDownload() throws IOException {
+    public void startBlockChainDownload() throws IOException {
+        // This does not need to be locked.
         setDownloadData(true);
         // TODO: peer might still have blocks that we don't have, and even have a heavier
         // chain even if the chain block count is lower.
-        if (getPeerBlockHeightDifference() >= 0) {
+        final int peerBlockHeightDifference = getPeerBlockHeightDifference();
+        if (peerBlockHeightDifference >= 0) {
             EventListenerInvoker.invoke(eventListeners, new EventListenerInvoker<PeerEventListener>() {
                 @Override
                 public void invoke(PeerEventListener listener) {
-                    listener.onChainDownloadStarted(Peer.this, getPeerBlockHeightDifference());
+                    listener.onChainDownloadStarted(Peer.this, peerBlockHeightDifference);
                 }
             });
 
@@ -1116,11 +1176,12 @@ public class Peer {
      * updated.
      * @throws ProtocolException if the peer version is too low to support measurable pings.
      */
-    public synchronized ListenableFuture<Long> ping() throws IOException, ProtocolException {
+    public ListenableFuture<Long> ping() throws IOException, ProtocolException {
         return ping((long) Math.random() * Long.MAX_VALUE);
     }
 
-    protected synchronized ListenableFuture<Long> ping(long nonce) throws IOException, ProtocolException {
+    protected ListenableFuture<Long> ping(long nonce) throws IOException, ProtocolException {
+        // This does not need to be locked.
         if (!getPeerVersionMessage().isPingPongSupported())
             throw new ProtocolException("Peer version is too low for measurable pings: " + getPeerVersionMessage());
         PendingPing pendingPing = new PendingPing(nonce);
@@ -1163,6 +1224,7 @@ public class Peer {
     }
 
     private void processPong(Pong m) {
+        // This does not need to be locked.
         PendingPing ping = null;
         // Iterates over a snapshot of the list, so we can run unlocked here.
         ListIterator<PendingPing> it = pendingPings.listIterator();
@@ -1182,14 +1244,19 @@ public class Peer {
      * Returns the difference between our best chain height and the peers, which can either be positive if we are
      * behind the peer, or negative if the peer is ahead of us.
      */
-    public synchronized int getPeerBlockHeightDifference() {
+    public int getPeerBlockHeightDifference() {
+        lock.lock();
+        try {
             // Chain will overflow signed int blocks in ~41,000 years.
             int chainHeight = (int) getBestHeight();
             // chainHeight should not be zero/negative because we shouldn't have given the user a Peer that is to another
             // client-mode node, nor should it be unconnected. If that happens it means the user overrode us somewhere or
             // there is a bug in the peer management code.
-        Preconditions.checkState(params.allowEmptyPeerChains || chainHeight > 0, "Connected to peer with zero/negative chain height", chainHeight);
+            checkState(params.allowEmptyPeerChains || chainHeight > 0, "Connected to peer with zero/negative chain height", chainHeight);
             return chainHeight - blockChain.getBestChainHeight();
+        } finally {
+            lock.unlock();
+        }
     }
 
     private boolean isNotFoundMessageSupported() {
@@ -1243,8 +1310,11 @@ public class Peer {
      * @return if not-null then this is the future for the Peer disconnection event.
      */
     public ChannelFuture setMinProtocolVersion(int minProtocolVersion) {
-        synchronized (this) {
+        lock.lock();
+        try {
             this.minProtocolVersion = minProtocolVersion;
+        } finally {
+            lock.unlock();
         }
         if (getVersionMessage().clientVersion < minProtocolVersion) {
             log.warn("{}: Disconnecting due to new min protocol version {}", this, minProtocolVersion);
@@ -1272,8 +1342,11 @@ public class Peer {
         if (!getPeerVersionMessage().isBloomFilteringSupported())
             return;
         boolean shouldQueryMemPool;
-        synchronized (this) {
+        lock.lock();
+        try {
             shouldQueryMemPool = memoryPool != null || downloadData.get();
+        } finally {
+            lock.unlock();
         }
         log.info("{}: Sending Bloom filter{}", this, shouldQueryMemPool ? " and querying mempool" : "");
         ChannelFuture future = sendMessage(filter);

core/src/main/java/com/google/bitcoin/core/PeerGroup.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2011 Google Inc.
+ * Copyright 2013 Google Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@ import com.google.bitcoin.core.Peer.PeerHandler;
 import com.google.bitcoin.discovery.PeerDiscovery;
 import com.google.bitcoin.discovery.PeerDiscoveryException;
 import com.google.bitcoin.utils.EventListenerInvoker;
+import com.google.bitcoin.utils.Locks;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.*;
@@ -39,9 +40,11 @@ import java.net.SocketAddress;
 import java.util.*;
 import java.util.concurrent.*;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 
 /**
  * <p>Runs a set of connections to the P2P network, brings up connections to replace disconnected nodes and manages
@@ -69,12 +72,12 @@ public class PeerGroup extends AbstractIdleService {
     private static final int DEFAULT_CONNECTIONS = 4;
 
     private static final Logger log = LoggerFactory.getLogger(PeerGroup.class);
+    protected final ReentrantLock lock = Locks.lock("peergroup");
 
     // These lists are all thread-safe so do not have to be accessed under the PeerGroup lock.
     // Addresses to try to connect to, excluding active peers.
     private List<PeerAddress> inactives;
-    // Currently active peers. This is an ordered list rather than a set to make unit tests predictable. This is a
+    // Currently active peers. This is an ordered list rather than a set to make unit tests predictable.
-    // synchronized list. Locking order is: PeerGroup < Peer < peers. Same for pendingPeers.
     private List<Peer> peers;
     // Currently connecting peers.
     private List<Peer> pendingPeers;
@@ -85,9 +88,9 @@ public class PeerGroup extends AbstractIdleService {
     // Callback for events related to chain download
     private PeerEventListener downloadListener;
     // Callbacks for events related to peer connection/disconnection
-    private List<PeerEventListener> peerEventListeners;
+    private final CopyOnWriteArrayList<PeerEventListener> peerEventListeners;
     // Peer discovery sources, will be polled occasionally if there aren't enough inactives.
-    private Set<PeerDiscovery> peerDiscoverers;
+    private CopyOnWriteArraySet<PeerDiscovery> peerDiscoverers;
     // The version message to use for new connections.
     private VersionMessage versionMessage;
     // A class that tracks recent transactions that have been broadcast across the network, counts how many
@@ -99,7 +102,7 @@ public class PeerGroup extends AbstractIdleService {
 
     // Runs a background thread that we use for scheduling pings to our peers, so we can measure their performance
     // and network latency. We ping peers every pingIntervalMsec milliseconds.
-    private Timer pingTimer;
+    private volatile Timer pingTimer;
     /** How many milliseconds to wait after receiving a pong before sending another ping. */
     public static final long DEFAULT_PING_INTERVAL_MSEC = 2000;
     private long pingIntervalMsec = DEFAULT_PING_INTERVAL_MSEC;
@@ -107,7 +110,7 @@ public class PeerGroup extends AbstractIdleService {
     private final NetworkParameters params;
     private final AbstractBlockChain chain;
     private long fastCatchupTimeSecs;
-    private ArrayList<Wallet> wallets;
+    private final CopyOnWriteArrayList<Wallet> wallets;
 
     private AbstractPeerEventListener getDataListener;
     private ClientBootstrap bootstrap;
@@ -190,7 +193,7 @@ public class PeerGroup extends AbstractIdleService {
         this.params = params;
         this.chain = chain;  // Can be null.
         this.fastCatchupTimeSecs = params.genesisBlock.getTimeSeconds();
-        this.wallets = new ArrayList<Wallet>(1);
+        this.wallets = new CopyOnWriteArrayList<Wallet>();
 
         // This default sentinel value will be overridden by one of two actions:
         //   - adding a peer discovery source sets it to the default
@@ -217,7 +220,7 @@ public class PeerGroup extends AbstractIdleService {
         pendingPeers = Collections.synchronizedList(new ArrayList<Peer>());
         channels = new DefaultChannelGroup();
         peerDiscoverers = new CopyOnWriteArraySet<PeerDiscovery>(); 
-        peerEventListeners = new ArrayList<PeerEventListener>();
+        peerEventListeners = new CopyOnWriteArrayList<PeerEventListener>();
         // This event listener is added to every peer. It's here so when we announce transactions via an "inv", every
         // peer can fetch them.
         getDataListener = new AbstractPeerEventListener() {
@@ -250,6 +253,7 @@ public class PeerGroup extends AbstractIdleService {
     private ChannelPipelineFactory makePipelineFactory(final NetworkParameters params, final AbstractBlockChain chain) {
         return new ChannelPipelineFactory() {
             public ChannelPipeline getPipeline() throws Exception {
+                // This runs unlocked.
                 VersionMessage ver = getVersionMessage().duplicate();
                 ver.bestHeight = chain == null ? 0 : chain.getBestChainHeight();
                 ver.time = Utils.now().getTime() / 1000;
@@ -274,9 +278,12 @@ public class PeerGroup extends AbstractIdleService {
      */
     public void setMaxConnections(int maxConnections) {
         int adjustment;
-        synchronized (this) {
+        lock.lock();
+        try {
             this.maxConnections = maxConnections;
             if (!isRunning()) return;
+        } finally {
+            lock.unlock();
         }
         // We may now have too many or too few open connections. Add more or drop some to get to the right amount.
         adjustment = maxConnections - channels.size();
@@ -295,13 +302,20 @@ public class PeerGroup extends AbstractIdleService {
     }
 
     /** The maximum number of connections that we will create to peers. */
-    public synchronized int getMaxConnections() {
+    public int getMaxConnections() {
+        lock.lock();
+        try {
             return maxConnections;
+        } finally {
+            lock.unlock();
+        }
     }
 
-    private synchronized List<Message> handleGetData(GetDataMessage m) {
+    private List<Message> handleGetData(GetDataMessage m) {
         // Scans the wallets and memory pool for transactions in the getdata message and returns them.
         // Runs on peer threads.
+        lock.lock();
+        try {
             LinkedList<Message> transactions = new LinkedList<Message>();
             LinkedList<InventoryItem> items = new LinkedList<InventoryItem>(m.getItems());
             Iterator<InventoryItem> it = items.iterator();
@@ -326,6 +340,9 @@ public class PeerGroup extends AbstractIdleService {
                 }
             }
             return transactions;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -337,15 +354,25 @@ public class PeerGroup extends AbstractIdleService {
      * The VersionMessage you provide is copied and the best chain height/time filled in for each new connection,
      * therefore you don't have to worry about setting that. The provided object is really more of a template.
      */
-    public synchronized void setVersionMessage(VersionMessage ver) {
+    public void setVersionMessage(VersionMessage ver) {
+        lock.lock();
+        try {
             versionMessage = ver;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
      * Returns the version message provided by setVersionMessage or a default if none was given.
      */
-    public synchronized VersionMessage getVersionMessage() {
+    public VersionMessage getVersionMessage() {
+        lock.lock();
+        try {
             return versionMessage;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -367,11 +394,16 @@ public class PeerGroup extends AbstractIdleService {
     }
     
     // Updates the relayTxesBeforeFilter flag of ver
-    private synchronized void updateVersionMessageRelayTxesBeforeFilter(VersionMessage ver) {
+    private void updateVersionMessageRelayTxesBeforeFilter(VersionMessage ver) {
         // We will provide the remote node with a bloom filter (ie they shouldn't relay yet)
         // iff chain == null || !chain.shouldVerifyTransactions() and a wallet is added
         // Note that the default here means that no tx invs will be received if no wallet is ever added
+        lock.lock();
+        try {
             ver.relayTxesBeforeFilter = chain != null && chain.shouldVerifyTransactions() && wallets.size() > 0;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -401,12 +433,12 @@ public class PeerGroup extends AbstractIdleService {
      * <p>The listener will be locked during callback execution, which in turn will cause network message processing
      * to stop until the listener returns.</p>
      */
-    public synchronized void addEventListener(PeerEventListener listener) {
+    public void addEventListener(PeerEventListener listener) {
         peerEventListeners.add(checkNotNull(listener));
     }
 
     /** The given event listener will no longer be called with events. */
-    public synchronized boolean removeEventListener(PeerEventListener listener) {
+    public boolean removeEventListener(PeerEventListener listener) {
         return peerEventListeners.remove(checkNotNull(listener));
     }
 
@@ -437,9 +469,12 @@ public class PeerGroup extends AbstractIdleService {
      */
     public void addAddress(PeerAddress peerAddress) {
         int newMax;
-        synchronized (this) {
+        lock.lock();
+        try {
             inactives.add(peerAddress);
             newMax = getMaxConnections() + 1;
+        } finally {
+            lock.unlock();
         }
         setMaxConnections(newMax);
     }
@@ -453,13 +488,19 @@ public class PeerGroup extends AbstractIdleService {
      * Add addresses from a discovery source to the list of potential peers to connect to. If max connections has not
      * been configured, or set to zero, then it's set to the default at this point.
      */
-    public synchronized void addPeerDiscovery(PeerDiscovery peerDiscovery) {
+    public void addPeerDiscovery(PeerDiscovery peerDiscovery) {
+        lock.lock();
+        try {
             if (getMaxConnections() == 0)
                 setMaxConnections(DEFAULT_CONNECTIONS);
             peerDiscoverers.add(peerDiscovery);
+        } finally {
+            lock.unlock();
+        }
     }
 
     protected void discoverPeers() throws PeerDiscoveryException {
+        // This does not need to be locked.
         long start = System.currentTimeMillis();
         Set<PeerAddress> addressSet = Sets.newHashSet();
         for (PeerDiscovery peerDiscovery : peerDiscoverers) {
@@ -477,9 +518,11 @@ public class PeerGroup extends AbstractIdleService {
 
     /** Picks a peer from discovery and connects to it. If connection fails, picks another and tries again. */
     protected void connectToAnyPeer() throws PeerDiscoveryException {
-        // Do not call this method whilst synchronized on the PeerGroup lock.
+        checkState(!lock.isLocked());
+        final State state = state();
+        if (!(state == State.STARTING || state == State.RUNNING)) return;
+
         final PeerAddress addr;
-        if (!(state() == State.STARTING || state() == State.RUNNING)) return;
         synchronized (inactives) {
             if (inactives.size() == 0) {
                 discoverPeers();
@@ -503,9 +546,7 @@ public class PeerGroup extends AbstractIdleService {
     @Override
     protected void startUp() throws Exception {
         // This is run in a background thread by the AbstractIdleService implementation.
-        synchronized (this) {
         pingTimer = new Timer("Peer pinging thread", true);
-        }
         // Bring up the requested number of connections. If a connect attempt fails,
         // new peers will be tried until there is a success, so just calling connectToAnyPeer for the wanted number
         // of peers is sufficient.
@@ -530,10 +571,8 @@ public class PeerGroup extends AbstractIdleService {
         for (PeerDiscovery peerDiscovery : peerDiscoverers) {
             peerDiscovery.shutdown();
         }
-        synchronized (this) {
         pingTimer.cancel();
     }
-    }
 
     /**
      * <p>Link the given wallet to this PeerGroup. This is used for three purposes:</p>
@@ -546,7 +585,9 @@ public class PeerGroup extends AbstractIdleService {
      * <p>Note that this should be done before chain download commences because if you add a wallet with keys earlier
      * than the current chain head, the relevant parts of the chain won't be redownloaded for you.</p>
      */
-    public synchronized void addWallet(Wallet wallet) {
+    public void addWallet(Wallet wallet) {
+        lock.lock();
+        try {
             Preconditions.checkNotNull(wallet);
             Preconditions.checkState(!wallets.contains(wallet));
             wallets.add(wallet);
@@ -559,14 +600,23 @@ public class PeerGroup extends AbstractIdleService {
             wallet.addEventListener(new AbstractWalletEventListener() {
                 @Override
                 public void onKeyAdded(ECKey key) {
+                    lock.lock();
+                    try {
                         recalculateFastCatchupAndFilter();
+                    } finally {
+                        lock.unlock();
+                    }
                 }
             });
             recalculateFastCatchupAndFilter();
             updateVersionMessageRelayTxesBeforeFilter(getVersionMessage());
+        } finally {
+            lock.unlock();
+        }
     }
 
-    private synchronized void recalculateFastCatchupAndFilter() {
+    private void recalculateFastCatchupAndFilter() {
+        checkState(lock.isLocked());
         // Fully verifying mode doesn't use this optimization (it can't as it needs to see all transactions).
         if (chain != null && chain.shouldVerifyTransactions())
             return;
@@ -606,18 +656,21 @@ public class PeerGroup extends AbstractIdleService {
      * See the docs for {@link BloomFilter#BloomFilter(int, double, long)} for a brief explanation of anonymity when
      * using bloom filters.
      */
-    public synchronized void setBloomFilterFalsePositiveRate(double bloomFilterFPRate) {
+    public void setBloomFilterFalsePositiveRate(double bloomFilterFPRate) {
+        lock.lock();
+        try {
             this.bloomFilterFPRate = bloomFilterFPRate;
             recalculateFastCatchupAndFilter();
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
      * Unlinks the given wallet so it no longer receives broadcast transactions or has its transactions announced.
      */
     public void removeWallet(Wallet wallet) {
-        if (wallet == null)
+        wallets.remove(checkNotNull(wallet));
-            throw new IllegalArgumentException("wallet is null");
-        wallets.remove(wallet);
     }
 
     /**
@@ -639,8 +692,9 @@ public class PeerGroup extends AbstractIdleService {
         return connectTo(address, true);
     }
 
-    // Internal version. Do not call whilst holding the PeerGroup lock.
+    // Internal version.
     protected ChannelFuture connectTo(SocketAddress address, boolean incrementMaxConnections) {
+        checkState(!lock.isLocked());
         ChannelFuture future = bootstrap.connect(address);
         // Make sure that the channel group gets access to the channel only if it connects successfully (otherwise
         // it cannot be closed and trying to do so will cause problems).
@@ -661,11 +715,14 @@ public class PeerGroup extends AbstractIdleService {
             // This can be null in unit tests or apps that don't use TCP connections.
             networkHandler.getOwnerObject().setRemoteAddress(address);
         }
-        synchronized (this) {
         if (incrementMaxConnections) {
             // We don't use setMaxConnections here as that would trigger a recursive attempt to establish a new
             // outbound connection.
+            lock.lock();
+            try {
                 maxConnections++;
+            } finally {
+                lock.unlock();
             }
         }
         return future;
@@ -687,7 +744,9 @@ public class PeerGroup extends AbstractIdleService {
      *
      * @param listener a listener for chain download events, may not be null
      */
-    public synchronized void startBlockChainDownload(PeerEventListener listener) {
+    public void startBlockChainDownload(PeerEventListener listener) {
+        lock.lock();
+        try {
             this.downloadListener = listener;
             // TODO: be more nuanced about which peer to download from.  We can also try
             // downloading from multiple peers and handle the case when a new peer comes along
@@ -697,6 +756,9 @@ public class PeerGroup extends AbstractIdleService {
                     startBlockChainDownloadFromPeer(peers.iterator().next());
                 }
             }
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -715,7 +777,9 @@ public class PeerGroup extends AbstractIdleService {
         }
     }
 
-    protected synchronized void handleNewPeer(final Peer peer) {
+    protected void handleNewPeer(final Peer peer) {
+        lock.lock();
+        try {
             // Runs on a netty worker thread for every peer that is newly connected. Peer is not locked at this point.
             // Sets up the newly connected peer so it can do everything it needs to.
             log.info("{}: New peer", peer);
@@ -724,7 +788,8 @@ public class PeerGroup extends AbstractIdleService {
             // OK because it helps improve wallet privacy. Old nodes will just ignore the message.
             try {
                 if (bloomFilter != null) peer.setBloomFilter(bloomFilter);
-        } catch (IOException e) { } // That was quick...already disconnected
+            } catch (IOException e) {
+            } // That was quick...already disconnected
             // Link the peer to the memory pool so broadcast transactions have their confidence levels updated.
             peer.setDownloadData(false);
             // TODO: The peer should calculate the fast catchup time from the added wallets here.
@@ -762,30 +827,35 @@ public class PeerGroup extends AbstractIdleService {
                     listener.onPeerConnected(peer, peers.size());
                 }
             });
-        final PeerGroup thisGroup = this;
             // TODO: Move this into the Peer object itself.
             peer.addEventListener(new AbstractPeerEventListener() {
                 int filteredBlocksReceivedFromPeer = 0;
+
                 @Override
                 public Message onPreMessageReceived(Peer peer, Message m) {
                     if (m instanceof FilteredBlock) {
                         filteredBlocksReceivedFromPeer++;
                         if (filteredBlocksReceivedFromPeer % RESEND_BLOOM_FILTER_BLOCK_COUNT == RESEND_BLOOM_FILTER_BLOCK_COUNT - 1) {
+                            lock.lock();
                             try {
-                            synchronized(thisGroup) {
                                 peer.sendMessage(bloomFilter);
-                            }
                             } catch (IOException e) {
                                 throw new RuntimeException(e);
+                            } finally {
+                                lock.unlock();
                             }
                         }
                     }
                     return m;
                 }
             });
+        } finally {
+            lock.unlock();
+        }
     }
 
     private void setupPingingForNewPeer(final Peer peer) {
+        checkState(lock.isLocked());
         if (peer.getPeerVersionMessage().clientVersion < Pong.MIN_PROTOCOL_VERSION)
             return;
         if (getPingIntervalMsec() <= 0)
@@ -831,6 +901,7 @@ public class PeerGroup extends AbstractIdleService {
     /** Returns true if at least one peer received an inv. */
     private synchronized boolean announcePendingWalletTransactions(List<Wallet> announceWallets,
                                                                    List<Peer> announceToPeers) {
+        checkState(lock.isLocked());
         // Build up an inv announcing the hashes of all pending transactions in all our wallets.
         InventoryMessage inv = new InventoryMessage(params);
         for (Wallet w : announceWallets) {
@@ -855,7 +926,9 @@ public class PeerGroup extends AbstractIdleService {
         return success;
     }
 
-    private synchronized void setDownloadPeer(Peer peer) {
+    private void setDownloadPeer(Peer peer) {
+        lock.lock();
+        try {
             if (downloadPeer == peer) {
                 return;
             }
@@ -875,6 +948,9 @@ public class PeerGroup extends AbstractIdleService {
                 downloadPeer.setDownloadData(true);
                 downloadPeer.setDownloadParameters(fastCatchupTimeSecs, bloomFilter != null);
             }
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -884,20 +960,24 @@ public class PeerGroup extends AbstractIdleService {
      * have that it's really valid.
      */
     public MemoryPool getMemoryPool() {
-        // Locking unneeded as memoryPool is final.
         return memoryPool;
     }
 
     /**
-     * Tells the PeerGroup to download only block headers before a certain time and bodies after that. See
+     * Tells the PeerGroup to download only block headers before a certain time and bodies after that. Call this
-     * {@link Peer#setFastCatchupTime(long)} for further explanation. Call this before starting block chain download.
+     * before starting block chain download.
      */
-    public synchronized void setFastCatchupTimeSecs(long secondsSinceEpoch) {
+    public void setFastCatchupTimeSecs(long secondsSinceEpoch) {
+        lock.lock();
+        try {
             Preconditions.checkState(chain == null || !chain.shouldVerifyTransactions(), "Fast catchup is incompatible with fully verifying");
             fastCatchupTimeSecs = secondsSinceEpoch;
             if (downloadPeer != null) {
                 downloadPeer.setDownloadParameters(secondsSinceEpoch, bloomFilter != null);
             }
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -906,8 +986,13 @@ public class PeerGroup extends AbstractIdleService {
      * the min of the wallets earliest key times.
      * @return a time in seconds since the epoch
      */
-    public synchronized long getFastCatchupTimeSecs() {
+    public long getFastCatchupTimeSecs() {
+        lock.lock();
+        try {
             return fastCatchupTimeSecs;
+        } finally {
+            lock.unlock();
+        }
     }
 
     protected void handlePeerDeath(final Peer peer) {
@@ -921,9 +1006,12 @@ public class PeerGroup extends AbstractIdleService {
         checkArgument(!peers.contains(peer));
         final Peer downloadPeer;
         final PeerEventListener downloadListener;
-        synchronized (this) {
+        lock.lock();
+        try {
             downloadPeer = this.downloadPeer;
             downloadListener = this.downloadListener;
+        } finally {
+            lock.unlock();
         }
         if (peer == downloadPeer) {
             log.info("Download peer died. Picking a new one.");
@@ -961,7 +1049,8 @@ public class PeerGroup extends AbstractIdleService {
         });
     }
 
-    private synchronized void startBlockChainDownloadFromPeer(Peer peer) {
+    private void startBlockChainDownloadFromPeer(Peer peer) {
+        lock.lock();
         try {
             peer.addEventListener(downloadListener);
             setDownloadPeer(peer);
@@ -969,7 +1058,8 @@ public class PeerGroup extends AbstractIdleService {
             peer.startBlockChainDownload();
         } catch (IOException e) {
             log.error("failed to start block chain download from " + peer, e);
-            return;
+        } finally {
+            lock.unlock();
         }
     }
 
@@ -1006,6 +1096,8 @@ public class PeerGroup extends AbstractIdleService {
      * @return
      */
     public int getMinBroadcastConnections() {
+        lock.lock();
+        try {
             if (minBroadcastConnections == 0) {
                 int max = getMaxConnections();
                 if (max <= 1)
@@ -1014,13 +1106,21 @@ public class PeerGroup extends AbstractIdleService {
                     return (int) Math.round(getMaxConnections() / 2.0);
             }
             return minBroadcastConnections;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
      * See {@link com.google.bitcoin.core.PeerGroup#getMinBroadcastConnections()}.
      */
     public void setMinBroadcastConnections(int value) {
+        lock.lock();
+        try {
             minBroadcastConnections = value;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -1076,7 +1176,8 @@ public class PeerGroup extends AbstractIdleService {
                             boolean done = false;
                             log.info("broadcastTransaction: TX {} seen by {} peers", pinnedTx.getHashAsString(),
                                     numSeenPeers);
-                            synchronized (PeerGroup.this) {
+                            lock.lock();
+                            try {
                                 if (numSeenPeers >= minConnections) {
                                     // We've seen the min required number of peers announce the transaction. Note that we
                                     // can't wait for the current number of connected peers right now because we could have
@@ -1101,6 +1202,8 @@ public class PeerGroup extends AbstractIdleService {
                                     }
                                     done = true;
                                 }
+                            } finally {
+                                lock.unlock();
                             }
                             if (done) {
                                 // We're done! Run this outside of the peer group lock as setting the future may immediately
@@ -1128,7 +1231,8 @@ public class PeerGroup extends AbstractIdleService {
                     if (minConnections == 1) {
                         sendComplete.addListener(new ChannelFutureListener() {
                             public void operationComplete(ChannelFuture _) throws Exception {
-                                synchronized (PeerGroup.this) {
+                                lock.lock();
+                                try {
                                     for (Wallet wallet : wallets) {
                                         try {
                                             if (wallet.isPendingTransactionRelevant(pinnedTx)) {
@@ -1141,6 +1245,8 @@ public class PeerGroup extends AbstractIdleService {
                                             return;
                                         }
                                     }
+                                } finally {
+                                    lock.unlock();
                                 }
                                 future.set(pinnedTx);
                                 return;
@@ -1161,8 +1267,13 @@ public class PeerGroup extends AbstractIdleService {
      * times are available via {@link com.google.bitcoin.core.Peer#getLastPingTime()} but it increases load on the
      * remote node. It defaults to 5000.
      */
-    public synchronized long getPingIntervalMsec() {
+    public long getPingIntervalMsec() {
+        lock.lock();
+        try {
             return pingIntervalMsec;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -1172,8 +1283,13 @@ public class PeerGroup extends AbstractIdleService {
      * Setting the value to be <= 0 disables pinging entirely, although you can still request one yourself
      * using {@link com.google.bitcoin.core.Peer#ping()}.
      */
-    public synchronized void setPingIntervalMsec(long pingIntervalMsec) {
+    public void setPingIntervalMsec(long pingIntervalMsec) {
+        lock.lock();
+        try {
             this.pingIntervalMsec = pingIntervalMsec;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
@@ -1181,7 +1297,7 @@ public class PeerGroup extends AbstractIdleService {
      * If no peers are connected, returns zero.
      */
     public int getMostCommonChainHeight() {
-        // Copy the peers list so we can calculate on it without violating lock ordering: Peer < peers
+        // Copy the peers list so we can calculate on it without violating lock ordering.
         ArrayList<Peer> peers;
         synchronized (this.peers) {
             peers = new ArrayList<Peer>(this.peers);
@@ -1307,6 +1423,11 @@ public class PeerGroup extends AbstractIdleService {
      * returns. Can return null if no peer was selected.
      */
     public Peer getDownloadPeer() {
+        lock.lock();
+        try {
             return downloadPeer;
+        } finally {
+            lock.unlock();
+        }
     }
 }

core/src/main/java/com/google/bitcoin/core/Utils.java

@@ -37,27 +37,13 @@ import static com.google.common.base.Preconditions.checkArgument;
  * To enable debug logging from the library, run with -Dbitcoinj.logging=true on your command line.
  */
 public class Utils {
-    public static final CycleDetectingLockFactory cycleDetectingLockFactory;
     private static final MessageDigest digest;
-
     static {
         try {
             digest = MessageDigest.getInstance("SHA-256");
         } catch (NoSuchAlgorithmException e) {
             throw new RuntimeException(e);  // Can't happen.
         }
-
-        cycleDetectingLockFactory = CycleDetectingLockFactory.newInstance(CycleDetectingLockFactory.Policies.THROW);
-    }
-
-    private static final boolean detectLockCycles = true;
-
-    /** Returns a re-entrant lock that may be cycle detecting, depending on {@link Utils#detectLockCycles}. */
-    public static ReentrantLock lock(String name) {
-        if (detectLockCycles)
-            return cycleDetectingLockFactory.newReentrantLock(name);
-        else
-            return new ReentrantLock();
     }
 
     /** The string that prefixes all text messages signed using Bitcoin keys. */

core/src/main/java/com/google/bitcoin/core/Wallet.java

@@ -20,6 +20,7 @@ import com.google.bitcoin.core.TransactionConfidence.ConfidenceType;
 import com.google.bitcoin.core.WalletTransaction.Pool;
 import com.google.bitcoin.store.WalletProtobufSerializer;
 import com.google.bitcoin.utils.EventListenerInvoker;
+import com.google.bitcoin.utils.Locks;
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Iterables;
@@ -74,7 +75,7 @@ public class Wallet implements Serializable, BlockChainListener {
     private static final Logger log = LoggerFactory.getLogger(Wallet.class);
     private static final long serialVersionUID = 2L;
 
-    protected final ReentrantLock lock = Utils.lock("wallet");
+    protected final ReentrantLock lock = Locks.lock("wallet");
 
     // Algorithm for movement of transactions between pools. Outbound tx = us spending coins. Inbound tx = us
     // receiving coins. If a tx is both inbound and outbound (spend with change) it is considered outbound for the
@@ -1388,8 +1389,9 @@ public class Wallet implements Serializable, BlockChainListener {
      * @param includeInactive If true, transactions that are on side chains (are unspendable) are included.
      */
     public Set<Transaction> getTransactions(boolean includeDead, boolean includeInactive) {
-        Set<Transaction> all = new HashSet<Transaction>();
         lock.lock();
+        try {
+            Set<Transaction> all = new HashSet<Transaction>();
             all.addAll(unspent.values());
             all.addAll(spent.values());
             all.addAll(pending.values());
@@ -1397,16 +1399,19 @@ public class Wallet implements Serializable, BlockChainListener {
                 all.addAll(dead.values());
             if (includeInactive)
                 all.addAll(inactive.values());
-        lock.unlock();
             return all;
+        } finally {
+            lock.unlock();
+        }
     }
 
     /**
      * Returns a set of all WalletTransactions in the wallet.
      */
     public Iterable<WalletTransaction> getWalletTransactions() {
-        HashSet<Transaction> pendingInactive = new HashSet<Transaction>();
         lock.lock();
+        try {
+            HashSet<Transaction> pendingInactive = new HashSet<Transaction>();
             pendingInactive.addAll(pending.values());
             pendingInactive.retainAll(inactive.values());
             HashSet<Transaction> onlyPending = new HashSet<Transaction>();
@@ -1424,8 +1429,10 @@ public class Wallet implements Serializable, BlockChainListener {
             addWalletTransactionsToSet(all, Pool.PENDING, onlyPending);
             addWalletTransactionsToSet(all, Pool.INACTIVE, onlyInactive);
             addWalletTransactionsToSet(all, Pool.PENDING_INACTIVE, pendingInactive);
-        lock.unlock();
             return all;
+        } finally {
+            lock.unlock();
+        }
     }
 
     private static void addWalletTransactionsToSet(Set<WalletTransaction> txs,
@@ -1452,7 +1459,7 @@ public class Wallet implements Serializable, BlockChainListener {
     /**
      * Adds the given transaction to the given pools and registers a confidence change listener on it.
      */
-    private synchronized void addWalletTransaction(Pool pool, Transaction tx) {
+    private void addWalletTransaction(Pool pool, Transaction tx) {
         checkState(lock.isLocked());
         switch (pool) {
         case UNSPENT:

core/src/main/java/com/google/bitcoin/store/SPVBlockStore.java

@@ -17,6 +17,7 @@
 package com.google.bitcoin.store;
 
 import com.google.bitcoin.core.*;
+import com.google.bitcoin.utils.Locks;
 import com.google.common.base.Preconditions;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -54,7 +55,7 @@ public class SPVBlockStore implements BlockStore {
     protected int numHeaders;
     protected NetworkParameters params;
 
-    protected ReentrantLock lock = Utils.lock("SPVBlockStore");
+    protected ReentrantLock lock = Locks.lock("SPVBlockStore");
 
     // The entire ring-buffer is mmapped and accessing it should be as fast as accessing regular memory once it's
     // faulted in. Unfortunately, in theory practice and theory are the same. In practice they aren't.

core/src/main/java/com/google/bitcoin/utils/Locks.java

@@ -0,0 +1,55 @@
+/**
+ * Copyright 2013 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.bitcoin.utils;
+
+import com.google.common.util.concurrent.CycleDetectingLockFactory;
+
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * A wrapper around explicit lock creation that lets you control whether bitcoinj performs cycle detection or not.
+ */
+public class Locks {
+
+    static {
+        // Default policy goes here. If you want to change this, use one of the static methods before
+        // instantiating any bitcoinj objects. The policy change will take effect only on new objects
+        // from that point onwards.
+        warnOnLockCycles();
+    }
+
+    public static CycleDetectingLockFactory factory = null;
+
+    public static ReentrantLock lock(String name) {
+        if (factory != null)
+            return factory.newReentrantLock(name);
+        else
+            return new ReentrantLock();
+    }
+
+    public static void warnOnLockCycles() {
+        factory = CycleDetectingLockFactory.newInstance(CycleDetectingLockFactory.Policies.WARN);
+    }
+
+    public static void throwOnLockCycles() {
+        factory = CycleDetectingLockFactory.newInstance(CycleDetectingLockFactory.Policies.THROW);
+    }
+
+    public static void ignoreLockCycles() {
+        factory = null;
+    }
+}

core/src/test/java/com/google/bitcoin/core/WalletTest.java

@@ -21,6 +21,7 @@ import com.google.bitcoin.core.WalletTransaction.Pool;
 import com.google.bitcoin.store.BlockStore;
 import com.google.bitcoin.store.MemoryBlockStore;
 import com.google.bitcoin.utils.BriefLogFormatter;
+import com.google.bitcoin.utils.Locks;
 import com.google.common.collect.Lists;
 import com.google.common.util.concurrent.CycleDetectingLockFactory;
 import org.junit.Before;
@@ -917,7 +918,9 @@ public class WalletTest {
 
     @Test
     public void lockCycles() {
-        final ReentrantLock lock = Utils.cycleDetectingLockFactory.newReentrantLock("test");
+        Locks.throwOnLockCycles();
+        final ReentrantLock lock = Locks.lock("test");
+        wallet = new Wallet(params);
         lock.lock();
         int foo = wallet.getKeychainSize();
         lock.unlock();