From ccbd30da8f96a247143514f7e860955904cea291 Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Fri, 14 Nov 2014 16:56:37 +0100 Subject: [PATCH] Delete !notfound code paths. --- .../src/main/java/org/bitcoinj/core/Peer.java | 27 ----- .../test/java/org/bitcoinj/core/PeerTest.java | 112 ++++-------------- 2 files changed, 24 insertions(+), 115 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/core/Peer.java b/core/src/main/java/org/bitcoinj/core/Peer.java index 40853504..afe274b1 100644 --- a/core/src/main/java/org/bitcoinj/core/Peer.java +++ b/core/src/main/java/org/bitcoinj/core/Peer.java @@ -130,10 +130,6 @@ public class Peer extends PeerSocketHandler { private static class GetDataRequest { Sha256Hash hash; SettableFuture future; - // If the peer does not support the notfound message, we'll use ping/pong messages to simulate it. This is - // a nasty hack that relies on the fact that bitcoin-qt is single threaded and processes messages in order. - // The nonce field records which pong should clear this request as "not found". - long nonce; } private final CopyOnWriteArrayList getDataFutures; @@ -744,7 +740,6 @@ public class Peer extends PeerSocketHandler { // Build the request for the missing dependencies. List> futures = Lists.newArrayList(); GetDataMessage getdata = new GetDataMessage(params); - final long nonce = (long)(Math.random()*Long.MAX_VALUE); if (needToRequest.size() > 1) log.info("{}: Requesting {} transactions for dep resolution", getAddress(), needToRequest.size()); for (Sha256Hash hash : needToRequest) { @@ -752,9 +747,6 @@ public class Peer extends PeerSocketHandler { GetDataRequest req = new GetDataRequest(); req.hash = hash; req.future = SettableFuture.create(); - if (!isNotFoundMessageSupported()) { - req.nonce = nonce; - } futures.add(req.future); getDataFutures.add(req); } @@ -803,25 +795,6 @@ public class Peer extends PeerSocketHandler { }); // Start the operation. sendMessage(getdata); - if (!isNotFoundMessageSupported()) { - // If the peer isn't new enough to support the notfound message, we use a nasty hack instead and - // assume if we send a ping message after the getdata message, it'll be processed after all answers - // from getdata are done, so we can watch for the pong message as a substitute. - log.info("{}: Dep resolution waiting for a pong with nonce {}", this, nonce); - ping(nonce).addListener(new Runnable() { - @Override - public void run() { - // The pong came back so clear out any transactions we requested but didn't get. - for (GetDataRequest req : getDataFutures) { - if (req.nonce == nonce) { - log.info("{}: Bottomed out dep tree at {}", this, req.hash); - req.future.cancel(true); - getDataFutures.remove(req); - } - } - } - }, Threading.SAME_THREAD); - } } catch (Exception e) { log.error("{}: Couldn't send getdata in downloadDependencies({})", this, tx.getHash()); resultFuture.setException(e); diff --git a/core/src/test/java/org/bitcoinj/core/PeerTest.java b/core/src/test/java/org/bitcoinj/core/PeerTest.java index b602e327..ef3e43db 100644 --- a/core/src/test/java/org/bitcoinj/core/PeerTest.java +++ b/core/src/test/java/org/bitcoinj/core/PeerTest.java @@ -533,18 +533,9 @@ public class PeerTest extends TestWithNetworkConnections { } @Test - public void recursiveDownloadNew() throws Exception { - recursiveDependencyDownload(true); - } - - @Test - public void recursiveDownloadOld() throws Exception { - recursiveDependencyDownload(false); - } - - public void recursiveDependencyDownload(boolean useNotFound) throws Exception { + public void recursiveDependencyDownload() throws Exception { // Using ping or notfound? - connectWithVersion(useNotFound ? 70001 : 60001); + connectWithVersion(70001); // Check that we can download all dependencies of an unconfirmed relevant transaction from the mempool. ECKey to = new ECKey(); @@ -604,8 +595,6 @@ public class PeerTest extends TestWithNetworkConnections { assertEquals(someHash, getdata.getItems().get(2).hash); assertEquals(anotherHash, getdata.getItems().get(3).hash); long nonce = -1; - if (!useNotFound) - nonce = ((Ping) outbound(writeTarget)).getNonce(); // For some random reason, t4 is delivered at this point before it's needed - perhaps it was a Bloom filter // false positive. We do this to check that the mempool is being checked for seen transactions before // requesting them. @@ -613,37 +602,25 @@ public class PeerTest extends TestWithNetworkConnections { // Deliver the requested transactions. inbound(writeTarget, t2); inbound(writeTarget, t3); - if (useNotFound) { - NotFoundMessage notFound = new NotFoundMessage(unitTestParams); - notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, someHash)); - notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, anotherHash)); - inbound(writeTarget, notFound); - } else { - inbound(writeTarget, new Pong(nonce)); - } + NotFoundMessage notFound = new NotFoundMessage(unitTestParams); + notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, someHash)); + notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, anotherHash)); + inbound(writeTarget, notFound); assertFalse(futures.isDone()); // It will recursively ask for the dependencies of t2: t5 and t4, but not t3 because it already found t4. getdata = (GetDataMessage) outbound(writeTarget); assertEquals(getdata.getItems().get(0).hash, t2.getInput(0).getOutpoint().getHash()); // t5 isn't found and t4 is. - if (useNotFound) { - NotFoundMessage notFound = new NotFoundMessage(unitTestParams); - notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t5)); - inbound(writeTarget, notFound); - } else { - bouncePing(); - } + notFound = new NotFoundMessage(unitTestParams); + notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t5)); + inbound(writeTarget, notFound); assertFalse(futures.isDone()); // Continue to explore the t4 branch and ask for t6, which is in the chain. getdata = (GetDataMessage) outbound(writeTarget); assertEquals(t6, getdata.getItems().get(0).hash); - if (useNotFound) { - NotFoundMessage notFound = new NotFoundMessage(unitTestParams); - notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t6)); - inbound(writeTarget, notFound); - } else { - bouncePing(); - } + notFound = new NotFoundMessage(unitTestParams); + notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t6)); + inbound(writeTarget, notFound); pingAndWait(writeTarget); // That's it, we explored the entire tree. assertTrue(futures.isDone()); @@ -653,23 +630,9 @@ public class PeerTest extends TestWithNetworkConnections { assertTrue(results.contains(t4)); } - private void bouncePing() throws Exception { - Ping ping = (Ping) outbound(writeTarget); - inbound(writeTarget, new Pong(ping.getNonce())); - } - @Test public void timeLockedTransactionNew() throws Exception { - timeLockedTransaction(true); - } - - @Test - public void timeLockedTransactionOld() throws Exception { - timeLockedTransaction(false); - } - - public void timeLockedTransaction(boolean useNotFound) throws Exception { - connectWithVersion(useNotFound ? 70001 : 60001); + connectWithVersion(70001); // Test that if we receive a relevant transaction that has a lock time, it doesn't result in a notification // until we explicitly opt in to seeing those. Wallet wallet = new Wallet(unitTestParams); @@ -686,11 +649,7 @@ public class PeerTest extends TestWithNetworkConnections { Transaction t1 = FakeTxBuilder.createFakeTx(unitTestParams, COIN, key); inbound(writeTarget, t1); GetDataMessage getdata = (GetDataMessage) outbound(writeTarget); - if (useNotFound) { - inbound(writeTarget, new NotFoundMessage(unitTestParams, getdata.getItems())); - } else { - bouncePing(); - } + inbound(writeTarget, new NotFoundMessage(unitTestParams, getdata.getItems())); pingAndWait(writeTarget); Threading.waitForUserCode(); assertNotNull(vtx[0]); @@ -705,45 +664,28 @@ public class PeerTest extends TestWithNetworkConnections { wallet.setAcceptRiskyTransactions(true); inbound(writeTarget, t2); getdata = (GetDataMessage) outbound(writeTarget); - if (useNotFound) { - inbound(writeTarget, new NotFoundMessage(unitTestParams, getdata.getItems())); - } else { - bouncePing(); - } + inbound(writeTarget, new NotFoundMessage(unitTestParams, getdata.getItems())); pingAndWait(writeTarget); Threading.waitForUserCode(); assertEquals(t2, vtx[0]); } @Test - public void rejectTimeLockedDependencyNew() throws Exception { + public void rejectTimeLockedDependency() throws Exception { // Check that we also verify the lock times of dependencies. Otherwise an attacker could still build a tx that // looks legitimate and useful but won't actually ever confirm, by sending us a normal tx that spends a // timelocked tx. - checkTimeLockedDependency(false, true); + checkTimeLockedDependency(false); } @Test - public void acceptTimeLockedDependencyNew() throws Exception { - checkTimeLockedDependency(true, true); + public void acceptTimeLockedDependency() throws Exception { + checkTimeLockedDependency(true); } - @Test - public void rejectTimeLockedDependencyOld() throws Exception { - // Check that we also verify the lock times of dependencies. Otherwise an attacker could still build a tx that - // looks legitimate and useful but won't actually ever confirm, by sending us a normal tx that spends a - // timelocked tx. - checkTimeLockedDependency(false, false); - } - - @Test - public void acceptTimeLockedDependencyOld() throws Exception { - checkTimeLockedDependency(true, false); - } - - private void checkTimeLockedDependency(boolean shouldAccept, boolean useNotFound) throws Exception { + private void checkTimeLockedDependency(boolean shouldAccept) throws Exception { // Initial setup. - connectWithVersion(useNotFound ? 70001 : 60001); + connectWithVersion(70001); Wallet wallet = new Wallet(unitTestParams); ECKey key = wallet.freshReceiveKey(); wallet.setAcceptRiskyTransactions(shouldAccept); @@ -780,19 +722,13 @@ public class PeerTest extends TestWithNetworkConnections { getdata = (GetDataMessage) outbound(writeTarget); assertEquals(t2.getHash(), getdata.getItems().get(0).hash); inbound(writeTarget, t2); - if (!useNotFound) - bouncePing(); // We request t3. getdata = (GetDataMessage) outbound(writeTarget); assertEquals(t3, getdata.getItems().get(0).hash); // Can't find it: bottom of tree. - if (useNotFound) { - NotFoundMessage notFound = new NotFoundMessage(unitTestParams); - notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t3)); - inbound(writeTarget, notFound); - } else { - bouncePing(); - } + NotFoundMessage notFound = new NotFoundMessage(unitTestParams); + notFound.addItem(new InventoryItem(InventoryItem.Type.Transaction, t3)); + inbound(writeTarget, notFound); pingAndWait(writeTarget); Threading.waitForUserCode(); // We're done but still not notified because it was timelocked.