From da8dba8b23114056f3031150b2f9ef2e6e2950ba Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Tue, 5 Jun 2012 12:26:41 +0200 Subject: [PATCH] Re-write block chain download handling to avoid parallel chain downloads occurring. Avoids big slowdowns when a block is solved during the chain download. Resolves issue 180. --- .../com/google/bitcoin/core/BlockChain.java | 52 +++++--- .../java/com/google/bitcoin/core/Peer.java | 101 +++++++++++----- .../com/google/bitcoin/core/PeerTest.java | 114 ++++++++++++------ 3 files changed, 182 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/BlockChain.java b/core/src/main/java/com/google/bitcoin/core/BlockChain.java index 32b1038e..fb4762b1 100644 --- a/core/src/main/java/com/google/bitcoin/core/BlockChain.java +++ b/core/src/main/java/com/google/bitcoin/core/BlockChain.java @@ -77,7 +77,7 @@ public class BlockChain { // Holds blocks that we have received but can't plug into the chain yet, eg because they were created whilst we // were downloading the block chain. - private final ArrayList unconnectedBlocks = new ArrayList(); + private final LinkedHashMap orphanBlocks = new LinkedHashMap(); /** * Constructs a BlockChain connected to the given wallet and store. To obtain a {@link Wallet} you can construct @@ -161,10 +161,9 @@ public class BlockChain { statsLastTime = System.currentTimeMillis(); statsBlocksAdded = 0; } - // We check only the chain head for double adds here to avoid potentially expensive block chain misses. - if (block.equals(getChainHead().getHeader())) { - // Duplicate add of the block at the top of the chain, can be a natural artifact of the download process. - log.debug("Chain head added more than once: {}", block.getHash()); + // Quick check for duplicates to avoid an expensive check further down (in findSplit). This can happen a lot + // when connecting orphan transactions due to the dumb brute force algorithm we use. + if (block.equals(getChainHead().getHeader()) || (tryConnecting && orphanBlocks.containsKey(block.getHash()))) { return true; } @@ -197,9 +196,9 @@ public class BlockChain { // We can't find the previous block. Probably we are still in the process of downloading the chain and a // block was solved whilst we were doing it. We put it to one side and try to connect it later when we // have more blocks. - checkState(tryConnecting, "bug in tryConnectingUnconnected"); + checkState(tryConnecting, "bug in tryConnectingOrphans"); log.warn("Block does not connect: {} prev {}", block.getHashAsString(), block.getPrevBlockHash()); - unconnectedBlocks.add(block); + orphanBlocks.put(block.getHash(), block); return false; } else { // It connects to somewhere on the chain. Not necessarily the top of the best known chain. @@ -213,7 +212,7 @@ public class BlockChain { } if (tryConnecting) - tryConnectingUnconnected(); + tryConnectingOrphans(); statsBlocksAdded++; return true; @@ -379,16 +378,19 @@ public class BlockChain { } /** - * For each block in unconnectedBlocks, see if we can now fit it on top of the chain and if so, do so. + * For each block in orphanBlocks, see if we can now fit it on top of the chain and if so, do so. */ - private void tryConnectingUnconnected() throws VerificationException, ScriptException, BlockStoreException { - // For each block in our unconnected list, try and fit it onto the head of the chain. If we succeed remove it + private void tryConnectingOrphans() throws VerificationException, ScriptException, BlockStoreException { + // For each block in our orphan list, try and fit it onto the head of the chain. If we succeed remove it // from the list and keep going. If we changed the head of the list at the end of the round try again until // we can't fit anything else on the top. + // + // This algorithm is kind of crappy, we should do a topo-sort then just connect them in order, but for small + // numbers of orphan blocks it does OK. int blocksConnectedThisRound; do { blocksConnectedThisRound = 0; - Iterator iter = unconnectedBlocks.iterator(); + Iterator iter = orphanBlocks.values().iterator(); while (iter.hasNext()) { Block block = iter.next(); log.debug("Trying to connect {}", block.getHash()); @@ -406,7 +408,7 @@ public class BlockChain { blocksConnectedThisRound++; } if (blocksConnectedThisRound > 0) { - log.info("Connected {} floating blocks.", blocksConnectedThisRound); + log.info("Connected {} orphan blocks.", blocksConnectedThisRound); } } while (blocksConnectedThisRound > 0); } @@ -565,12 +567,26 @@ public class BlockChain { } /** - * Returns the most recent unconnected block or null if there are none. This will all have to change. It's used - * only in processing of inv messages. + * An orphan block is one that does not connect to the chain anywhere (ie we can't find its parent, therefore + * it's an orphan). Typically this occurs when we are downloading the chain and didn't reach the head yet, and/or + * if a block is solved whilst we are downloading. It's possible that we see a small amount of orphan blocks which + * chain together, this method tries walking backwards through the known orphan blocks to find the bottom-most. + * + * @return from or one of froms parents, or null if "from" does not identify an orphan block */ - synchronized Block getUnconnectedBlock() { - if (unconnectedBlocks.size() == 0) + public synchronized Block getOrphanRoot(Sha256Hash from) { + Block cursor = orphanBlocks.get(from); + if (cursor == null) return null; - return unconnectedBlocks.get(unconnectedBlocks.size() - 1); + Block tmp; + while ((tmp = orphanBlocks.get(cursor.getPrevBlockHash())) != null) { + cursor = tmp; + } + return cursor; + } + + /** Returns true if the given block is currently in the orphan blocks list. */ + public synchronized boolean isOrphan(Sha256Hash block) { + return orphanBlocks.containsKey(block); } } diff --git a/core/src/main/java/com/google/bitcoin/core/Peer.java b/core/src/main/java/com/google/bitcoin/core/Peer.java index 9608099f..126af7c3 100644 --- a/core/src/main/java/com/google/bitcoin/core/Peer.java +++ b/core/src/main/java/com/google/bitcoin/core/Peer.java @@ -19,15 +19,13 @@ package com.google.bitcoin.core; import com.google.bitcoin.store.BlockStore; import com.google.bitcoin.store.BlockStoreException; import com.google.bitcoin.utils.EventListenerInvoker; +import com.google.common.base.Objects; import com.google.common.base.Preconditions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.util.ArrayList; -import java.util.Iterator; -import java.util.LinkedList; -import java.util.List; +import java.util.*; import java.util.concurrent.*; /** @@ -67,6 +65,13 @@ public class Peer { // is set AND our best block is before that date, switch to false until block headers beyond that point have been // received at which point it gets set to true again. This isn't relevant unless downloadData is true. private boolean downloadBlockBodies = true; + // Keeps track of things we requested internally with getdata but didn't receive yet, so we can avoid re-requests. + // It's not quite the same as pendingGetBlockFutures, as this is used only for getdatas done as part of downloading + // the chain and so is lighter weight (we just keep a bunch of hashes not futures). + // + // It is important to avoid a nasty edge case where we can end up with parallel chain downloads proceeding + // simultaneously if we were to receive a newly solved block whilst parts of the chain are streaming to us. + private HashSet pendingBlockDownloads = new HashSet(); /** * Construct a peer that reads/writes from the given block chain. Note that communication won't occur until @@ -338,20 +343,29 @@ public class Peer { log.warn("Received block we did not ask for: {}", m.getHashAsString()); return; } + pendingBlockDownloads.remove(m.getHash()); // Otherwise it's a block sent to us because the peer thought we needed it, so add it to the block chain. // This call will synchronize on blockChain. if (blockChain.add(m)) { // The block was successfully linked into the chain. Notify the user of our progress. invokeOnBlocksDownloaded(m); } else { - // This block is unconnected - we don't know how to get from it back to the genesis block yet. That + // This block is an orphan - we don't know how to get from it back to the genesis block yet. That // must mean that there are blocks we are missing, so do another getblocks with a new block locator // to ask the peer to send them to us. This can happen during the initial block chain download where // the peer will only send us 500 at a time and then sends us the head block expecting us to request // the others. - - // TODO: Should actually request root of orphan chain here. - blockChainDownload(m.getHash()); + // + // We must do two things here: + // (1) Request from current top of chain to the root of the current set of orphan blocks and + // (2) Filter out duplicate getblock requests (done in blockChainDownload). + // + // The reason for (1) is that otherwise if new blocks were solved during the middle of chain download + // we'd do a blockChainDownload() on the new best chain head, which would cause us to try and grab the + // chain twice (or more!) on the same connection! The block chain would filter out the duplicates but + // only at a huge speed penalty. By finding the orphan root we ensure every getblocks looks the same + // no matter how many blocks are solved, and therefore that the (2) duplicate filtering can work. + blockChainDownload(blockChain.getOrphanRoot(m.getHash()).getHash()); } } catch (VerificationException e) { // We don't want verification failures to kill the thread. @@ -416,18 +430,37 @@ public class Peer { } if (blocks.size() > 0 && downloadData) { - Block topBlock = blockChain.getUnconnectedBlock(); - Sha256Hash topHash = (topBlock != null ? topBlock.getHash() : null); - if (isNewBlockTickle(topHash, blocks)) { - // An inv with a single hash containing our most recent unconnected block is a special inv, - // it's kind of like a tickle from the peer telling us that it's time to download more blocks to catch up to - // the block chain. We could just ignore this and treat it as a regular inv but then we'd download the head - // block over and over again after each batch of 500 blocks, which is wasteful. - blockChainDownload(topHash); - return; + // Ideally, we'd only ask for the data here if we actually needed it. However that can imply a lot of + // disk IO to figure out what we've got. Normally peers will not send us inv for things we already have + // so we just re-request it here, and if we get duplicates the block chain / wallet will filter them out. + for (InventoryItem item : blocks) { + if (blockChain.isOrphan(item.hash)) { + // If an orphan was re-advertised, ask for more blocks. + blockChainDownload(blockChain.getOrphanRoot(item.hash).getHash()); + } else { + // Don't re-request blocks we already requested. Normally this should not happen. However there is + // an edge case: if a block is solved and we complete the inv<->getdata<->block<->getblocks cycle + // whilst other parts of the chain are streaming in, then the new getblocks request won't match the + // previous one: whilst the stopHash is the same (because we use the orphan root), the start hash + // will be different and so the getblocks req won't be dropped as a duplicate. We'll end up + // requesting a subset of what we already requested, which can lead to parallel chain downloads + // and other nastyness. So we just do a quick removal of redundant getdatas here too. + // + // Note that as of June 2012 the Satoshi client won't actually ever interleave blocks pushed as + // part of chain download with newly announced blocks, so it should always be taken care of by + // the duplicate check in blockChainDownload(). But the satoshi client may change in future so + // it's better to be safe here. + if (!pendingBlockDownloads.contains(item.hash)) { + getdata.addItem(item); + pendingBlockDownloads.add(item.hash); + } + } } - // Request the advertised blocks only if we're the download peer. - for (InventoryItem item : blocks) getdata.addItem(item); + // If we're downloading the chain, doing a getdata on the last block we were told about will cause the + // peer to advertize the head block to us in a single-item inv. When we download THAT, it will be an + // orphan block, meaning we'll re-enter blockChainDownload() to trigger another getblocks between the + // current best block we have and the orphan block. If more blocks arrive in the meantime they'll also + // become orphan. } if (!getdata.getItems().isEmpty()) { @@ -436,14 +469,6 @@ public class Peer { } } - /** A new block tickle is an inv with a hash containing the topmost block. */ - private boolean isNewBlockTickle(Sha256Hash topHash, List items) { - return items.size() == 1 && - items.get(0).type == InventoryItem.Type.Block && - topHash != null && - items.get(0).hash.equals(topHash); - } - /** * Asks the connected peer for the block of the given hash, and returns a Future representing the answer. * If you want the block right away and don't mind waiting for it, just call .get() on the result. Your thread @@ -563,6 +588,10 @@ public class Peer { conn.writeMessage(m); } + // Keep track of the last request we made to the peer in blockChainDownload so we can avoid redundant and harmful + // getblocks requests. + private Sha256Hash lastGetBlocksBegin, lastGetBlocksEnd; + private void blockChainDownload(Sha256Hash toHash) throws IOException { // This may run in ANY thread. @@ -609,8 +638,15 @@ public class Peer { // 50 block headers. If there is a re-org deeper than that, we'll end up downloading the entire chain. We // must always put the genesis block as the first entry. BlockStore store = blockChain.getBlockStore(); - StoredBlock cursor = blockChain.getChainHead(); - log.info("blockChainDownload({}) current head = ", toHash.toString(), cursor); + StoredBlock chainHead = blockChain.getChainHead(); + Sha256Hash chainHeadHash = chainHead.getHeader().getHash(); + // Did we already make this request? If so, don't do it again. + if (Objects.equal(lastGetBlocksBegin, chainHeadHash) && Objects.equal(lastGetBlocksEnd, toHash)) { + log.info("blockChainDownload({}): ignoring duplicated request", toHash.toString()); + return; + } + log.info("blockChainDownload({}) current head = {}", toHash.toString(), chainHead.getHeader().getHashAsString()); + StoredBlock cursor = chainHead; for (int i = 50; cursor != null && i > 0; i--) { blockLocator.add(cursor.getHeader().getHash()); try { @@ -625,8 +661,11 @@ public class Peer { blockLocator.add(params.genesisBlock.getHash()); } - // The toHash field is set to zero already by the constructor. This is how we indicate "never stop". - + // Record that we requested this range of blocks so we can filter out duplicate requests in the event of a + // block being solved during chain download. + lastGetBlocksBegin = chainHeadHash; + lastGetBlocksEnd = toHash; + if (downloadBlockBodies) { GetBlocksMessage message = new GetBlocksMessage(params, blockLocator, toHash); conn.writeMessage(message); diff --git a/core/src/test/java/com/google/bitcoin/core/PeerTest.java b/core/src/test/java/com/google/bitcoin/core/PeerTest.java index 2d6fb3b4..f7ad8b77 100644 --- a/core/src/test/java/com/google/bitcoin/core/PeerTest.java +++ b/core/src/test/java/com/google/bitcoin/core/PeerTest.java @@ -25,6 +25,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.Future; +import static com.google.bitcoin.core.TestUtils.*; import static org.easymock.EasyMock.*; import static org.junit.Assert.*; @@ -80,36 +81,77 @@ public class PeerTest extends TestWithNetworkConnections { runPeer(peer, conn); } - // Check that when we receive a block that does not connect to our chain, we send a getblocks to fetch - // the intermediates. @Test - public void unconnectedBlock() throws Exception { - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + public void chainDownloadEnd2End() throws Exception { + // A full end-to-end test of the chain download process, with a new block being solved in the middle. + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); - blockChain.add(b2); // b2 is top block. - Block b3 = TestUtils.makeSolvedTestBlock(unitTestParams, b2); - Block b4 = TestUtils.makeSolvedTestBlock(unitTestParams, b3); - conn.inbound(b4); - runPeer(peer, conn); - GetBlocksMessage getblocks = (GetBlocksMessage) conn.popOutbound(); - List expectedLocator = new ArrayList(); - // Locator contains top block (b2), prev hash (b1), the genesis block. - expectedLocator.add(b2.getHash()); - expectedLocator.add(b2.getPrevBlockHash()); - expectedLocator.add(unitTestParams.genesisBlock.getHash()); - assertEquals(expectedLocator, getblocks.getLocator()); - assertEquals(b4.getHash(), getblocks.getStopHash()); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); + Block b3 = makeSolvedTestBlock(unitTestParams, b2); + Block b4 = makeSolvedTestBlock(unitTestParams, b3); + Block b5 = makeSolvedTestBlock(unitTestParams, b4); + conn.setVersionMessageForHeight(unitTestParams, 6); + peer.startBlockChainDownload(); + runPeerAsync(peer, conn); + GetBlocksMessage getblocks = (GetBlocksMessage) conn.outbound(); + assertEquals(blockStore.getChainHead().getHeader().getHash(), getblocks.getLocator().get(0)); + assertEquals(Sha256Hash.ZERO_HASH, getblocks.getStopHash()); + // Remote peer sends us an inv with some blocks. + InventoryMessage inv = new InventoryMessage(unitTestParams); + inv.addBlock(b2); + inv.addBlock(b3); + // We do a getdata on them. + GetDataMessage getdata = (GetDataMessage) conn.exchange(inv); + assertEquals(b2.getHash(), getdata.getItems().get(0).hash); + assertEquals(b3.getHash(), getdata.getItems().get(1).hash); + assertEquals(2, getdata.getItems().size()); + // Remote peer sends us the blocks. The act of doing a getdata for b3 results in getting an inv with just the + // best chain head in it. + conn.inbound(b2); + conn.inbound(b3); + inv = new InventoryMessage(unitTestParams); + inv.addBlock(b5); + // We request the head block. + getdata = (GetDataMessage) conn.exchange(inv); + assertEquals(b5.getHash(), getdata.getItems().get(0).hash); + assertEquals(1, getdata.getItems().size()); + // Peer sends us the head block. The act of receiving the orphan block triggers a getblocks to fill in the + // rest of the chain. + getblocks = (GetBlocksMessage) conn.exchange(b5); + assertEquals(b5.getHash(), getblocks.getStopHash()); + assertEquals(b3.getHash(), getblocks.getLocator().get(0)); + // At this point another block is solved and broadcast. The inv triggers a getdata but we do NOT send another + // getblocks afterwards, because that would result in us receiving the same set of blocks twice which is a + // timewaste. The getblocks message that would have been generated is set to be the same as the previous + // because we walk backwards down the orphan chain and then discover we already asked for those blocks, so + // nothing is done. + Block b6 = makeSolvedTestBlock(unitTestParams, b5); + inv = new InventoryMessage(unitTestParams); + inv.addBlock(b6); + getdata = (GetDataMessage) conn.exchange(inv); + assertEquals(1, getdata.getItems().size()); + assertEquals(b6.getHash(), getdata.getItems().get(0).hash); + assertNull(conn.exchange(b6)); // Nothing is sent at this point. + // We're still waiting for the response to the getblocks (b3,b5) sent above. + inv = new InventoryMessage(unitTestParams); + inv.addBlock(b4); + inv.addBlock(b5); + getdata = (GetDataMessage) conn.exchange(inv); + assertEquals(1, getdata.getItems().size()); + assertEquals(b4.getHash(), getdata.getItems().get(0).hash); + // We already have b5 from before, so it's not requested again. + assertNull(conn.exchange(b4)); + // b5 and b6 are now connected by the block chain and we're done. } // Check that an inventory tickle is processed correctly when downloading missing blocks is active. @Test public void invTickle() throws Exception { - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); // Make a missing block. - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); - Block b3 = TestUtils.makeSolvedTestBlock(unitTestParams, b2); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); + Block b3 = makeSolvedTestBlock(unitTestParams, b2); conn.inbound(b3); InventoryMessage inv = new InventoryMessage(unitTestParams); InventoryItem item = new InventoryItem(InventoryItem.Type.Block, b3.getHash()); @@ -132,9 +174,9 @@ public class PeerTest extends TestWithNetworkConnections { peer.setDownloadData(false); // Make a missing block that we receive. - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); // Receive an inv. InventoryMessage inv = new InventoryMessage(unitTestParams); @@ -152,7 +194,7 @@ public class PeerTest extends TestWithNetworkConnections { peer.setDownloadData(true); // Make a transaction and tell the peer we have it. BigInteger value = Utils.toNanoCoins(1, 0); - Transaction tx = TestUtils.createFakeTx(unitTestParams, value, address); + Transaction tx = createFakeTx(unitTestParams, value, address); InventoryMessage inv = new InventoryMessage(unitTestParams); InventoryItem item = new InventoryItem(InventoryItem.Type.Transaction, tx.getHash()); inv.addItem(item); @@ -182,7 +224,7 @@ public class PeerTest extends TestWithNetworkConnections { // Make a tx and advertise it to one of the peers. BigInteger value = Utils.toNanoCoins(1, 0); - Transaction tx = TestUtils.createFakeTx(unitTestParams, value, address); + Transaction tx = createFakeTx(unitTestParams, value, address); InventoryMessage inv = new InventoryMessage(unitTestParams); InventoryItem item = new InventoryItem(InventoryItem.Type.Transaction, tx.getHash()); inv.addItem(item); @@ -209,9 +251,9 @@ public class PeerTest extends TestWithNetworkConnections { PeerEventListener listener = control.createMock(PeerEventListener.class); peer.addEventListener(listener); - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); conn.setVersionMessageForHeight(unitTestParams, 100); // Receive notification of a new block. InventoryMessage inv = new InventoryMessage(unitTestParams); @@ -243,9 +285,9 @@ public class PeerTest extends TestWithNetworkConnections { PeerEventListener listener = control.createMock(PeerEventListener.class); peer.addEventListener(listener); - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); blockChain.add(b2); conn.setVersionMessageForHeight(unitTestParams, 100); @@ -269,10 +311,10 @@ public class PeerTest extends TestWithNetworkConnections { @Test public void getBlock() throws Exception { - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); - Block b3 = TestUtils.makeSolvedTestBlock(unitTestParams, b2); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); + Block b3 = makeSolvedTestBlock(unitTestParams, b2); conn.setVersionMessageForHeight(unitTestParams, 100); runPeerAsync(peer, conn); // Request the block. @@ -293,14 +335,14 @@ public class PeerTest extends TestWithNetworkConnections { public void fastCatchup() throws Exception { // Check that blocks before the fast catchup point are retrieved using getheaders, and after using getblocks. // This test is INCOMPLETE because it does not check we handle >2000 blocks correctly. - Block b1 = TestUtils.createFakeBlock(unitTestParams, blockStore).block; + Block b1 = createFakeBlock(unitTestParams, blockStore).block; blockChain.add(b1); Utils.rollMockClock(60 * 10); // 10 minutes later. - Block b2 = TestUtils.makeSolvedTestBlock(unitTestParams, b1); + Block b2 = makeSolvedTestBlock(unitTestParams, b1); Utils.rollMockClock(60 * 10); // 10 minutes later. - Block b3 = TestUtils.makeSolvedTestBlock(unitTestParams, b2); + Block b3 = makeSolvedTestBlock(unitTestParams, b2); Utils.rollMockClock(60 * 10); - Block b4 = TestUtils.makeSolvedTestBlock(unitTestParams, b3); + Block b4 = makeSolvedTestBlock(unitTestParams, b3); conn.setVersionMessageForHeight(unitTestParams, 4); // Request headers until the last 2 blocks. peer.setFastCatchupTime((Utils.now().getTime() / 1000) - (600*2) + 1);