From 590d47f273dd98a7dd83f40ec110c0c5abc93398 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Tue, 29 Jan 2013 16:17:46 +0100 Subject: [PATCH] Add Wallets to all peers not just the download peer. Resolves issue 297. Also fix the unit test that was meant to catch this error so it didn't accidentally probe the download peer case. And prevent adding of wallets multiple times (which caught another error in the unit tests). --- .../com/google/bitcoin/core/PeerGroup.java | 11 ++++--- .../google/bitcoin/core/PeerGroupTest.java | 32 +++++++++---------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/PeerGroup.java b/core/src/main/java/com/google/bitcoin/core/PeerGroup.java index 3977a2c7..fb3f48b4 100644 --- a/core/src/main/java/com/google/bitcoin/core/PeerGroup.java +++ b/core/src/main/java/com/google/bitcoin/core/PeerGroup.java @@ -541,6 +541,7 @@ public class PeerGroup extends AbstractIdleService { */ public synchronized void addWallet(Wallet wallet) { Preconditions.checkNotNull(wallet); + Preconditions.checkState(!wallets.contains(wallet)); wallets.add(wallet); announcePendingWalletTransactions(Collections.singletonList(wallet), peers); @@ -718,6 +719,9 @@ public class PeerGroup extends AbstractIdleService { // Link the peer to the memory pool so broadcast transactions have their confidence levels updated. peer.setMemoryPool(memoryPool); peer.setDownloadData(false); + // TODO: The peer should calculate the fast catchup time from the added wallets here. + for (Wallet wallet : wallets) + peer.addWallet(wallet); // Re-evaluate download peers. Peer newDownloadPeer = selectDownloadPeer(peers); if (downloadPeer != newDownloadPeer) { @@ -853,17 +857,12 @@ public class PeerGroup extends AbstractIdleService { if (downloadPeer != null) { log.info("Unsetting download peer: {}", downloadPeer); downloadPeer.setDownloadData(false); - for (Wallet wallet : wallets) - downloadPeer.removeWallet(wallet); } downloadPeer = peer; if (downloadPeer != null) { log.info("Setting download peer: {}", downloadPeer); downloadPeer.setDownloadData(true); downloadPeer.setDownloadParameters(fastCatchupTimeSecs, bloomFilter != null); - // TODO: The peer should calculate the fast catchup time from the added wallets here. - for (Wallet wallet : wallets) - downloadPeer.addWallet(wallet); } } @@ -941,6 +940,8 @@ public class PeerGroup extends AbstractIdleService { } // TODO: Remove peerEventListeners from the Peer here. peer.removeEventListener(getDataListener); + for (Wallet wallet : wallets) + peer.removeWallet(wallet); EventListenerInvoker.invoke(peerEventListeners, new EventListenerInvoker() { @Override public void invoke(PeerEventListener listener) { diff --git a/core/src/test/java/com/google/bitcoin/core/PeerGroupTest.java b/core/src/test/java/com/google/bitcoin/core/PeerGroupTest.java index 18f6b479..9c9ff9f0 100644 --- a/core/src/test/java/com/google/bitcoin/core/PeerGroupTest.java +++ b/core/src/test/java/com/google/bitcoin/core/PeerGroupTest.java @@ -19,9 +19,6 @@ package com.google.bitcoin.core; import com.google.bitcoin.discovery.PeerDiscovery; import com.google.bitcoin.discovery.PeerDiscoveryException; import com.google.bitcoin.store.MemoryBlockStore; - -import org.jboss.netty.bootstrap.ClientBootstrap; -import org.jboss.netty.channel.*; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -94,7 +91,6 @@ public class PeerGroupTest extends TestWithPeerGroup { peerGroup.startAndWait(); // Create a couple of peers. - peerGroup.addWallet(wallet); FakeChannel p1 = connectPeer(1); FakeChannel p2 = connectPeer(2); @@ -111,19 +107,22 @@ public class PeerGroupTest extends TestWithPeerGroup { InventoryMessage inv = new InventoryMessage(unitTestParams); inv.addTransaction(t1); - inbound(p1, inv); - assertTrue(outbound(p1) instanceof BloomFilter); - assertTrue(outbound(p1) instanceof GetDataMessage); + // Note: we start with p2 here to verify that transactions are downloaded from whichever peer announces first + // which does not have to be the same as the download peer (which is really the "block download peer"). inbound(p2, inv); assertTrue(outbound(p2) instanceof BloomFilter); - assertNull(outbound(p2)); // Only one peer is used to download. - inbound(p1, t1); - assertNull(outbound(p2)); + assertTrue(outbound(p2) instanceof GetDataMessage); + inbound(p1, inv); + assertTrue(outbound(p1) instanceof BloomFilter); + assertNull(outbound(p1)); // Only one peer is used to download. + inbound(p2, t1); + assertNull(outbound(p1)); // Asks for dependency. - GetDataMessage getdata = (GetDataMessage) outbound(p1); - inbound(p1, new NotFoundMessage(unitTestParams, getdata.getItems())); + GetDataMessage getdata = (GetDataMessage) outbound(p2); + assertNotNull(getdata); + inbound(p2, new NotFoundMessage(unitTestParams, getdata.getItems())); assertEquals(value, wallet.getBalance(Wallet.BalanceType.ESTIMATED)); - peerGroup.stop(); + peerGroup.stopAndWait(); } @Test @@ -220,19 +219,20 @@ public class PeerGroupTest extends TestWithPeerGroup { InventoryMessage inv = new InventoryMessage(params); inv.addTransaction(tx); - // Peer 2 advertises the tx but does not download it. + // Peer 2 advertises the tx but does not receive it yet. inbound(p2, inv); assertTrue(outbound(p2) instanceof BloomFilter); assertTrue(outbound(p2) instanceof GetDataMessage); assertEquals(0, tx.getConfidence().numBroadcastPeers()); assertTrue(peerGroup.getMemoryPool().maybeWasSeen(tx.getHash())); assertNull(event[0]); - // Peer 1 advertises the tx, we don't do anything as it's already been requested. + // Peer 1 advertises the tx, we don't do anything as it's already been requested. inbound(p1, inv); assertTrue(outbound(p1) instanceof BloomFilter); assertNull(outbound(p1)); + // Peer 2 gets sent the tx and requests the dependency. inbound(p2, tx); - assertNull(outbound(p2)); + assertTrue(outbound(p2) instanceof GetDataMessage); tx = event[0]; // We want to use the canonical copy delivered by the PeerGroup from now on. assertNotNull(tx); event[0] = null;