Bloom: don't requery mempool if the filter is only being refreshed to…

… force down FP rate.
bitcoinj · May 28, 2014 · b36bb5b · b36bb5b
1 parent ecbaaf0
commit b36bb5b
Show file tree

Hide file tree

Showing 2 changed files with 34 additions and 11 deletions.
diff --git a/core/src/main/java/com/google/bitcoin/core/Peer.java b/core/src/main/java/com/google/bitcoin/core/Peer.java
@@ -1480,15 +1480,33 @@ public boolean setMinProtocolVersion(int minProtocolVersion) {
      * unset a filter, though the underlying p2p protocol does support it.</p>
      */
     public void setBloomFilter(BloomFilter filter) {
+        setBloomFilter(filter, memoryPool != null || vDownloadData);
+    }
+
+    /**
+     * <p>Sets a Bloom filter on this connection. This will cause the given {@link BloomFilter} object to be sent to the
+     * remote peer and if requested, a {@link MemoryPoolMessage} is sent as well to trigger downloading of any
+     * pending transactions that may be relevant.</p>
+     *
+     * <p>The Peer does not automatically request filters from any wallets added using {@link Peer#addWallet(Wallet)}.
+     * This is to allow callers to avoid redundantly recalculating the same filter repeatedly when using multiple peers
+     * and multiple wallets together.</p>
+     *
+     * <p>Therefore, you should not use this method if your app uses a {@link PeerGroup}. It is called for you.</p>
+     *
+     * <p>If the remote peer doesn't support Bloom filtering, then this call is ignored. Once set you presently cannot
+     * unset a filter, though the underlying p2p protocol does support it.</p>
+     */
+    public void setBloomFilter(BloomFilter filter, boolean andQueryMemPool) {
         checkNotNull(filter, "Clearing filters is not currently supported");
         final VersionMessage ver = vPeerVersionMessage;
         if (ver == null || !ver.isBloomFilteringSupported())
             return;
         vBloomFilter = filter;
-        boolean shouldQueryMemPool = memoryPool != null || vDownloadData;
-        log.info("{}: Sending Bloom filter{}", this, shouldQueryMemPool ? " and querying mempool" : "");
+        log.info("{}: Sending Bloom filter{}", this, andQueryMemPool ? " and querying mempool" : "");
         sendMessage(filter);
-        sendMessage(new MemoryPoolMessage());
+        if (andQueryMemPool)
+            sendMessage(new MemoryPoolMessage());
     }
 
     /**

diff --git a/core/src/main/java/com/google/bitcoin/core/PeerGroup.java b/core/src/main/java/com/google/bitcoin/core/PeerGroup.java
@@ -139,10 +139,11 @@ public List<Message> getData(Peer peer, GetDataMessage m) {
 
         @Override
         public void onBlocksDownloaded(Peer peer, Block block, int blocksLeft) {
-            double rate = checkNotNull(chain).getFalsePositiveRate();
-            if (rate > bloomFilterMerger.getBloomFilterFPRate() * MAX_FP_RATE_INCREASE) {
-                log.info("Force update Bloom filter due to high false positive rate");
-                recalculateFastCatchupAndFilter(FilterRecalculateMode.FORCE_SEND);
+            final double rate = checkNotNull(chain).getFalsePositiveRate();
+            final double target = bloomFilterMerger.getBloomFilterFPRate() * MAX_FP_RATE_INCREASE;
+            if (rate > target) {
+                log.info("Force update Bloom filter due to high false positive rate ({} vs {})", rate, target);
+                recalculateFastCatchupAndFilter(FilterRecalculateMode.FORCE_SEND_FOR_REFRESH);
             }
         }
     };
@@ -846,7 +847,7 @@ public void removeWallet(Wallet wallet) {
 
     public static enum FilterRecalculateMode {
         SEND_IF_CHANGED,
-        FORCE_SEND,
+        FORCE_SEND_FOR_REFRESH,
         DONT_SEND,
     }
 
@@ -868,12 +869,16 @@ public void recalculateFastCatchupAndFilter(FilterRecalculateMode mode) {
             switch (mode) {
                 case SEND_IF_CHANGED: send = result.changed; break;
                 case DONT_SEND: send = false; break;
-                case FORCE_SEND: send = true; break;
+                case FORCE_SEND_FOR_REFRESH: send = true; break;
                 default: throw new UnsupportedOperationException();
             }
             if (send) {
-                for (Peer peer : peers)
-                    peer.setBloomFilter(result.filter);
+                for (Peer peer : peers) {
+                    // Only query the mempool if this recalculation request is not in order to lower the observed FP
+                    // rate. There's no point querying the mempool when doing this because the FP rate can only go
+                    // down, and we will have seen all the relevant txns before: it's pointless to ask for them again.
+                    peer.setBloomFilter(result.filter, mode != FilterRecalculateMode.FORCE_SEND_FOR_REFRESH);
+                }
                 // Reset the false positive estimate so that we don't send a flood of filter updates
                 // if the estimate temporarily overshoots our threshold.
                 if (chain != null)