From 40c4338aaac230b9674371ff972dc4f6d44695c8 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Sun, 7 Dec 2014 13:17:35 +0200 Subject: [PATCH] Some fixes for crashes that could occur with a chain-less PeerGroup post-TxConfidenceTable changes. --- .../main/java/org/bitcoinj/core/Context.java | 7 +++-- .../java/org/bitcoinj/core/PeerGroup.java | 31 +++++++++---------- .../bitcoinj/core/TransactionBroadcast.java | 9 ++++-- .../java/org/bitcoinj/core/PeerGroupTest.java | 2 +- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/Context.java b/core/src/main/java/org/bitcoinj/core/Context.java index c5401e39..d03bed10 100644 --- a/core/src/main/java/org/bitcoinj/core/Context.java +++ b/core/src/main/java/org/bitcoinj/core/Context.java @@ -1,9 +1,10 @@ package org.bitcoinj.core; /** - * The Context object holds various objects that are relevant to the global state of our - * view of the Bitcoin network. You can get an instance of this class - * through {@link AbstractBlockChain#getContext()}. + * The Context object holds various objects that are scoped to a specific instantiation of bitcoinj for a specific + * network. You can get an instance of this class through {@link AbstractBlockChain#getContext()}. At the momemnt it + * only contains a {@link org.bitcoinj.core.TxConfidenceTable} but in future it will likely contain file paths and + * other global configuration of use. */ public class Context { protected TxConfidenceTable confidenceTable; diff --git a/core/src/main/java/org/bitcoinj/core/PeerGroup.java b/core/src/main/java/org/bitcoinj/core/PeerGroup.java index 78641075..677f89c2 100644 --- a/core/src/main/java/org/bitcoinj/core/PeerGroup.java +++ b/core/src/main/java/org/bitcoinj/core/PeerGroup.java @@ -119,9 +119,6 @@ public class PeerGroup implements TransactionBroadcaster { @GuardedBy("lock") private VersionMessage versionMessage; // Switch for enabling download of pending transaction dependencies. @GuardedBy("lock") private boolean downloadTxDependencies; - // A class that tracks recent transactions that have been broadcast across the network, counts how many - // peers announced them and updates the transaction confidence data. It is passed to each Peer. - private final TxConfidenceTable confidenceTable; // How many connections we want to have open at the current time. If we lose connections, we'll try opening more // until we reach this count. @GuardedBy("lock") private int maxConnections; @@ -139,7 +136,7 @@ public class PeerGroup implements TransactionBroadcaster { @GuardedBy("lock") private boolean ipv6Unreachable = false; private final NetworkParameters params; - private final AbstractBlockChain chain; + @Nullable private final AbstractBlockChain chain; @GuardedBy("lock") private long fastCatchupTimeSecs; private final CopyOnWriteArrayList wallets; private final CopyOnWriteArrayList peerFilterProviders; @@ -154,7 +151,8 @@ public class PeerGroup implements TransactionBroadcaster { @Override public void onBlocksDownloaded(Peer peer, Block block, int blocksLeft) { - final double rate = checkNotNull(chain).getFalsePositiveRate(); + if (chain == null) return; + final double rate = chain.getFalsePositiveRate(); final double target = bloomFilterMerger.getBloomFilterFPRate() * MAX_FP_RATE_INCREASE; if (rate > target) { // TODO: Avoid hitting this path if the remote peer didn't acknowledge applying a new filter yet. @@ -335,8 +333,6 @@ public class PeerGroup implements TransactionBroadcaster { downloadTxDependencies = true; - confidenceTable = chain.getContext().getConfidenceTable(); - inactives = new PriorityQueue(1, new Comparator() { @SuppressWarnings("FieldAccessNotGuarded") // only called when inactives is accessed, and lock is held then. @Override @@ -450,8 +446,8 @@ public class PeerGroup implements TransactionBroadcaster { } } - long retryTime = 0; - PeerAddress addrToTry = null; + long retryTime; + PeerAddress addrToTry; lock.lock(); try { if (doDiscovery) { @@ -521,7 +517,7 @@ public class PeerGroup implements TransactionBroadcaster { while (it.hasNext()) { InventoryItem item = it.next(); // Check the confidence pool first. - Transaction tx = confidenceTable.get(item.hash); + Transaction tx = chain != null ? chain.getContext().getConfidenceTable().get(item.hash) : null; if (tx != null) { transactions.add(tx); it.remove(); @@ -1326,11 +1322,13 @@ public class PeerGroup implements TransactionBroadcaster { } /** - * Use {@link org.bitcoinj.core.Context#getConfidenceTable()} instead. + * Use {@link org.bitcoinj.core.Context#getConfidenceTable()} instead, which can be retrieved via + * {@link org.bitcoinj.core.AbstractBlockChain#getContext()}. Can return null if this peer group was + * configured without a block chain object. */ - @Deprecated - public TxConfidenceTable getConfidenceTable() { - return confidenceTable; + @Deprecated @Nullable + public TxConfidenceTable getMemoryPool() { + return chain == null ? null : chain.getContext().getConfidenceTable(); } /** @@ -1341,7 +1339,7 @@ public class PeerGroup implements TransactionBroadcaster { public void setFastCatchupTimeSecs(long secondsSinceEpoch) { lock.lock(); try { - Preconditions.checkState(chain == null || !chain.shouldVerifyTransactions(), "Fast catchup is incompatible with fully verifying"); + checkState(chain == null || !chain.shouldVerifyTransactions(), "Fast catchup is incompatible with fully verifying"); fastCatchupTimeSecs = secondsSinceEpoch; if (downloadPeer != null) { downloadPeer.setDownloadParameters(secondsSinceEpoch, bloomFilterMerger.getLastFilter() != null); @@ -1563,7 +1561,8 @@ public class PeerGroup implements TransactionBroadcaster { * bringup of the peer group you can lower it.

*/ public ListenableFuture broadcastTransaction(final Transaction tx, final int minConnections) { - final TransactionBroadcast broadcast = new TransactionBroadcast(this, tx); + // TODO: Context being owned by BlockChain isn't right w.r.t future intentions so it shouldn't really be optional here. + final TransactionBroadcast broadcast = new TransactionBroadcast(this, chain != null ? chain.getContext() : null, tx); broadcast.setMinConnections(minConnections); // Send the TX to the wallet once we have a successful broadcast. Futures.addCallback(broadcast.future(), new FutureCallback() { diff --git a/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java b/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java index 30962956..74e47f5f 100644 --- a/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java +++ b/core/src/main/java/org/bitcoinj/core/TransactionBroadcast.java @@ -24,6 +24,7 @@ import com.google.common.util.concurrent.SettableFuture; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.annotation.Nullable; import java.util.Collections; import java.util.List; import java.util.Random; @@ -41,6 +42,7 @@ public class TransactionBroadcast { private final SettableFuture future = SettableFuture.create(); private final PeerGroup peerGroup; private final Transaction tx; + @Nullable private final Context context; private int minConnections; private int numWaitingFor, numToBroadcastTo; @@ -49,8 +51,10 @@ public class TransactionBroadcast { public static Random random = new Random(); private Transaction pinnedTx; - public TransactionBroadcast(PeerGroup peerGroup, Transaction tx) { + // TODO: Context being owned by BlockChain isn't right w.r.t future intentions so it shouldn't really be optional here. + TransactionBroadcast(PeerGroup peerGroup, @Nullable Context context, Transaction tx) { this.peerGroup = peerGroup; + this.context = context; this.tx = tx; this.minConnections = Math.max(1, peerGroup.getMinBroadcastConnections()); } @@ -98,7 +102,8 @@ public class TransactionBroadcast { // a big effect. List peers = peerGroup.getConnectedPeers(); // snapshots // We intern the tx here so we are using a canonical version of the object (as it's unfortunately mutable). - pinnedTx = peerGroup.getConfidenceTable().intern(tx); + // TODO: Once confidence state is moved out of Transaction we can kill off this step. + pinnedTx = context != null ? context.getConfidenceTable().intern(tx) : pinnedTx; // Prepare to send the transaction by adding a listener that'll be called when confidence changes. // Only bother with this if we might actually hear back: if (minConnections > 1) diff --git a/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java b/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java index 890818c2..67746bd4 100644 --- a/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java +++ b/core/src/test/java/org/bitcoinj/core/PeerGroupTest.java @@ -376,7 +376,7 @@ public class PeerGroupTest extends TestWithPeerGroup { inbound(p2, inv); assertTrue(outbound(p2) instanceof GetDataMessage); assertEquals(0, tx.getConfidence().numBroadcastPeers()); - assertTrue(peerGroup.getConfidenceTable().maybeWasSeen(tx.getHash())); + assertTrue(blockChain.getContext().getConfidenceTable().maybeWasSeen(tx.getHash())); assertNull(event[0]); // Peer 1 advertises the tx, we don't do anything as it's already been requested. inbound(p1, inv);