diff --git a/core/src/main/java/com/google/bitcoin/core/PeerFilterProvider.java b/core/src/main/java/com/google/bitcoin/core/PeerFilterProvider.java
index abbdfb88..2f1fe3f8 100644
--- a/core/src/main/java/com/google/bitcoin/core/PeerFilterProvider.java
+++ b/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();
}
diff --git a/core/src/main/java/com/google/bitcoin/core/Wallet.java b/core/src/main/java/com/google/bitcoin/core/Wallet.java
index 7b07fd9e..de96e04f 100644
--- a/core/src/main/java/com/google/bitcoin/core/Wallet.java
+++ b/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 not 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;
+ }
}
diff --git a/core/src/main/java/com/google/bitcoin/net/FilterMerger.java b/core/src/main/java/com/google/bitcoin/net/FilterMerger.java
index a4f4bdce..75df856e 100644
--- a/core/src/main/java/com/google/bitcoin/net/FilterMerger.java
+++ b/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,36 +37,52 @@ public class FilterMerger {
}
public Result calculate(ImmutableList providers) {
- Result result = new Result();
- result.earliestKeyTimeSecs = Long.MAX_VALUE;
- int elements = 0;
- boolean requiresUpdateAll = false;
- for (PeerFilterProvider p : providers) {
- result.earliestKeyTimeSecs = Math.min(result.earliestKeyTimeSecs, p.getEarliestKeyCreationTime());
- elements += p.getBloomFilterElementCount();
- requiresUpdateAll = requiresUpdateAll || p.isRequiringUpdateAllBloomFilter();
- }
+ LinkedList takenLocks = new LinkedList();
+ 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;
+ boolean requiresUpdateAll = false;
+ for (PeerFilterProvider p : providers) {
+ result.earliestKeyTimeSecs = Math.min(result.earliestKeyTimeSecs, p.getEarliestKeyCreationTime());
+ elements += p.getBloomFilterElementCount();
+ requiresUpdateAll = requiresUpdateAll || p.isRequiringUpdateAllBloomFilter();
+ }
- if (elements > 0) {
- // We stair-step our element count so that we avoid creating a filter with different parameters
- // as much as possible as that results in a loss of privacy.
- // The constant 100 here is somewhat arbitrary, but makes sense for small to medium wallets -
- // it will likely mean we never need to create a filter with different parameters.
- lastBloomFilterElementCount = elements > lastBloomFilterElementCount ? elements + 100 : lastBloomFilterElementCount;
- BloomFilter.BloomUpdate bloomFlags =
- requiresUpdateAll ? BloomFilter.BloomUpdate.UPDATE_ALL : BloomFilter.BloomUpdate.UPDATE_P2PUBKEY_ONLY;
- BloomFilter filter = new BloomFilter(lastBloomFilterElementCount, bloomFilterFPRate, bloomFilterTweak, bloomFlags);
- for (PeerFilterProvider p : providers)
- filter.merge(p.getBloomFilter(lastBloomFilterElementCount, bloomFilterFPRate, bloomFilterTweak));
+ if (elements > 0) {
+ // We stair-step our element count so that we avoid creating a filter with different parameters
+ // as much as possible as that results in a loss of privacy.
+ // The constant 100 here is somewhat arbitrary, but makes sense for small to medium wallets -
+ // it will likely mean we never need to create a filter with different parameters.
+ lastBloomFilterElementCount = elements > lastBloomFilterElementCount ? elements + 100 : lastBloomFilterElementCount;
+ BloomFilter.BloomUpdate bloomFlags =
+ requiresUpdateAll ? BloomFilter.BloomUpdate.UPDATE_ALL : BloomFilter.BloomUpdate.UPDATE_P2PUBKEY_ONLY;
+ BloomFilter filter = new BloomFilter(lastBloomFilterElementCount, bloomFilterFPRate, bloomFilterTweak, bloomFlags);
+ for (PeerFilterProvider p : providers)
+ filter.merge(p.getBloomFilter(lastBloomFilterElementCount, bloomFilterFPRate, bloomFilterTweak));
- result.changed = !filter.equals(lastFilter);
- result.filter = lastFilter = filter;
+ result.changed = !filter.equals(lastFilter);
+ result.filter = lastFilter = filter;
+ }
+ // Now adjust the earliest key time backwards by a week to handle the case of clock drift. This can occur
+ // both in block header timestamps and if the users clock was out of sync when the key was first created
+ // (to within a small amount of tolerance).
+ result.earliestKeyTimeSecs -= 86400 * 7;
+ return result;
+ } finally {
+ for (Lock takenLock : takenLocks) {
+ takenLock.unlock();
+ }
}
- // Now adjust the earliest key time backwards by a week to handle the case of clock drift. This can occur
- // both in block header timestamps and if the users clock was out of sync when the key was first created
- // (to within a small amount of tolerance).
- result.earliestKeyTimeSecs -= 86400 * 7;
- return result;
}
public void setBloomFilterFPRate(double bloomFilterFPRate) {
diff --git a/core/src/test/java/com/google/bitcoin/core/BitcoindComparisonTool.java b/core/src/test/java/com/google/bitcoin/core/BitcoindComparisonTool.java
index 54bd4961..eca8935e 100644
--- a/core/src/test/java/com/google/bitcoin/core/BitcoindComparisonTool.java
+++ b/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();