Require PeerFilterProviders to expose a lock, and use them to avoid a race that occurs during Bloom filter construction.

It's possible in some uses to cause the Wallet to create two batches of keys one after the other, but independently such that the second batch of keys is derived (with HDWs) whilst the filter is being recalculated from the first. This in turn could race with filter calculation and cause asserts or miscalculated filters.

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

@@ -16,6 +16,8 @@
 
 package com.google.bitcoin.core;
 
+import java.util.concurrent.locks.Lock;
+
 /**
  * An interface which provides the information required to properly filter data downloaded from Peers.
  * Note that an implementer is responsible for calling {@link PeerGroup#recalculateFastCatchupAndFilter(boolean)} whenever a
@@ -43,4 +45,15 @@ public interface PeerFilterProvider {
 
     /** Whether this filter provider depends on the server updating the filter on all matches */
     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 com.google.bitcoin.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();
 }

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

@@ -3834,4 +3834,17 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi
         }.start();
         return rekeyTx;
     }
+
+    /**
+     * Returns the wallet lock under which most operations happen. This is here to satisfy the
+     * {@link com.google.bitcoin.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.
+     */
+    public ReentrantLock getLock() {
+        return lock;
+    }
 }

core/src/main/java/com/google/bitcoin/net/FilterMerger.java

@@ -4,6 +4,9 @@ import com.google.bitcoin.core.BloomFilter;
 import com.google.bitcoin.core.PeerFilterProvider;
 import com.google.common.collect.ImmutableList;
 
+import java.util.LinkedList;
+import java.util.concurrent.locks.Lock;
+
 // This code is unit tested by the PeerGroup tests.
 
 /**
@@ -34,6 +37,17 @@ public class FilterMerger {
     }
 
     public Result calculate(ImmutableList<PeerFilterProvider> providers) {
+        LinkedList<Lock> takenLocks = new LinkedList<Lock>();
+        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.
+            for (PeerFilterProvider provider : providers) {
+                Lock lock = provider.getLock();
+                lock.lock();
+                takenLocks.add(lock);
+            }
             Result result = new Result();
             result.earliestKeyTimeSecs = Long.MAX_VALUE;
             int elements = 0;
@@ -64,6 +78,11 @@ public class FilterMerger {
             // (to within a small amount of tolerance).
             result.earliestKeyTimeSecs -= 86400 * 7;
             return result;
+        } finally {
+            for (Lock takenLock : takenLocks) {
+                takenLock.unlock();
+            }
+        }
     }
 
     public void setBloomFilterFPRate(double bloomFilterFPRate) {

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

@@ -31,6 +31,7 @@ import java.io.File;
 import java.net.InetAddress;
 import java.util.*;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.Lock;
 
 /**
  * A tool for comparing the blocks which are accepted/rejected by bitcoind/bitcoinj
@@ -120,6 +121,8 @@ 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;
             }
@@ -133,6 +136,11 @@ public class BitcoindComparisonTool {
                 return false;
             }
 
+            @Override
+            public Lock getLock() {
+                return lock;
+            }
+
             @Override public BloomFilter getBloomFilter(int size, double falsePositiveRate, long nTweak) {
                 BloomFilter filter = new BloomFilter(1, 0.99, 0);
                 filter.setMatchAll();