Change the PeerFilterProvider interface to have begin/end methods instead of requiring a lock to be exposed. It's more efficient and flexible.

core/src/main/java/org/bitcoinj/core/PeerFilterProvider.java

@@ -31,6 +31,16 @@ public interface PeerFilterProvider {
      */
     public long getEarliestKeyCreationTime();
 
+    /**
+     * Called on all registered filter providers before getBloomFilterElementCount and getBloomFilter are called.
+     * Once called, the provider should ensure that the items it will want to insert into the filter don't change.
+     * The reason is that all providers will have their element counts queried, and then a filter big enough for
+     * all of them will be specified. So the provider must use consistent state. There is guaranteed to be a matching
+     * call to endBloomFilterCalculation that can be used to e.g. unlock a lock.
+     */
+    public void beginBloomFilterCalculation();
+
+
     /**
      * Gets the number of elements that will be added to a bloom filter returned by
      * {@link PeerFilterProvider#getBloomFilter(int, double, long)}
@@ -46,14 +56,5 @@ public interface PeerFilterProvider {
     /** Whether this filter provider depends on the server updating the filter on all matches */
     public boolean isRequiringUpdateAllBloomFilter();
 
-    /**
-     * Returns an object that will be locked before any other methods are called and unlocked afterwards. You must
-     * provide one of these because the results from calling the above methods must be consistent. Otherwise it's
-     * possible for the {@link org.bitcoinj.net.FilterMerger} to request the counts of a bunch of providers
-     * with {@link #getBloomFilterElementCount()}, create a filter of the right size, call {@link #getBloomFilter(int, double, long)}
-     * and then the filter provider discovers it's been mutated in the mean time and now has a different number of
-     * elements. For instance, a Wallet that has keys added to it whilst a filter recalc is in progress could cause
-     * experience this race.
-     */
-    public Lock getLock();
+    public void endBloomFilterCalculation();
 }

core/src/main/java/org/bitcoinj/core/Wallet.java

@@ -3818,6 +3818,18 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
 
     //region Bloom filtering
 
+    @Override
+    public void beginBloomFilterCalculation() {
+        lock.lock();
+        keychainLock.readLock().lock();
+    }
+
+    @Override
+    public void endBloomFilterCalculation() {
+        keychainLock.readLock().unlock();
+        lock.unlock();
+    }
+
     /**
      * Returns the number of distinct data items (note: NOT keys) that will be inserted into a bloom filter, when it
      * is constructed.
@@ -4516,50 +4528,4 @@ public class Wallet extends BaseTaggableObject implements Serializable, BlockCha
         }
     }
     //endregion
-
-    /**
-     * Returns the wallet lock under which most operations happen. This is here to satisfy the
-     * {@link org.bitcoinj.core.PeerFilterProvider} interface and generally should not be used directly by apps.
-     * In particular, do <b>not</b> hold this lock if you're display a send confirm screen to the user or for any other
-     * long length of time, as it may cause processing holdups elsewhere. Instead, for the "confirm payment screen"
-     * use case you should complete a candidate transaction, present it to the user (e.g. for fee purposes) and then
-     * when they confirm - which may be quite some time later - recalculate the transaction and check if it's the same.
-     * If not, redisplay the confirm window and try again.
-     */
-    @Override
-    public Lock getLock() {
-        return new Lock() {
-            @Override
-            public void lock() {
-                lock.lock();
-                keychainLock.readLock().lock();
-            }
-
-            @Override
-            public void lockInterruptibly() throws InterruptedException {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public boolean tryLock() {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public boolean tryLock(long l, TimeUnit unit) throws InterruptedException {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public void unlock() {
-                keychainLock.readLock().unlock();
-                lock.unlock();
-            }
-
-            @Override
-            public Condition newCondition() {
-                throw new UnsupportedOperationException();
-            }
-        };
-    }
 }

core/src/main/java/org/bitcoinj/net/FilterMerger.java

@@ -1,5 +1,6 @@
 package org.bitcoinj.net;
 
+import com.google.common.collect.Lists;
 import org.bitcoinj.core.BloomFilter;
 import org.bitcoinj.core.PeerFilterProvider;
 import com.google.common.collect.ImmutableList;
@@ -37,16 +38,15 @@ public class FilterMerger {
     }
 
     public Result calculate(ImmutableList<PeerFilterProvider> providers) {
-        LinkedList<Lock> takenLocks = new LinkedList<Lock>();
+        LinkedList<PeerFilterProvider> begunProviders = Lists.newLinkedList();
         try {
-            // Lock all the providers so they cannot be mutated out from underneath us whilst we're in the process
-            // of calculating the Bloom filter. All providers must be in a consistent, unchanging state because the
-            // filter is a merged one that's large enough for all providers elements: if a provider were to get more
-            // elements in the middle of the calculation, we might assert or calculate the filter wrongly.
+            // All providers must be in a consistent, unchanging state because the filter is a merged one that's
+            // large enough for all providers elements: if a provider were to get more elements in the middle of the
+            // calculation, we might assert or calculate the filter wrongly. Most providers use a lock here but
+            // snapshotting required state is also a legitimate strategy.
             for (PeerFilterProvider provider : providers) {
-                Lock lock = provider.getLock();
-                lock.lock();
-                takenLocks.add(lock);
+                provider.beginBloomFilterCalculation();
+                begunProviders.add(provider);
             }
             Result result = new Result();
             result.earliestKeyTimeSecs = Long.MAX_VALUE;
@@ -79,8 +79,8 @@ public class FilterMerger {
             result.earliestKeyTimeSecs -= 86400 * 7;
             return result;
         } finally {
-            for (Lock takenLock : takenLocks) {
-                takenLock.unlock();
+            for (PeerFilterProvider provider : begunProviders) {
+                provider.endBloomFilterCalculation();
             }
         }
     }

core/src/test/java/org/bitcoinj/core/BitcoindComparisonTool.java

@@ -121,8 +121,6 @@ public class BitcoindComparisonTool {
             }
         }, Threading.SAME_THREAD);
         peers.addPeerFilterProvider(new PeerFilterProvider() {
-            private final Lock lock = Threading.lock("pfp");
-
             @Override public long getEarliestKeyCreationTime() {
                 return Long.MAX_VALUE;
             }
@@ -137,8 +135,11 @@ public class BitcoindComparisonTool {
             }
 
             @Override
-            public Lock getLock() {
-                return lock;
+            public void beginBloomFilterCalculation() {
+            }
+
+            @Override
+            public void endBloomFilterCalculation() {
             }
 
             @Override public BloomFilter getBloomFilter(int size, double falsePositiveRate, long nTweak) {