Fix an issue in block chain handling, whereby a duplicate block received that was not the chain head could result in wallet corruption and bogus "block forks the chain" messages. Resolves issue 135.

src/com/google/bitcoin/core/BlockChain.java

@@ -153,6 +153,7 @@ public class BlockChain {
         // We check only the chain head for double adds here to avoid potentially expensive block chain misses.
         if (block.equals(chainHead.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());
             return true;
         }
 
@@ -187,6 +188,7 @@ 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.
+            assert tryConnecting : "bug in tryConnectingUnconnected";
             log.warn("Block does not connect: {}", block.getHashAsString());
             unconnectedBlocks.add(block);
             return false;
@@ -227,14 +229,24 @@ public class BlockChain {
                 log.info("Block is causing a re-organize");
             } else {
                 StoredBlock splitPoint = findSplit(newStoredBlock, chainHead);
+                if (splitPoint == newStoredBlock) {
+                    // newStoredBlock is a part of the same chain, there's no fork. This happens when we receive a block
+                    // that we already saw and linked into the chain previously, which isn't the chain head.
+                    // Re-processing it is confusing for the wallet so just skip.
+                    log.debug("Saw duplicated block in main chain at height {}: {}",
+                            newStoredBlock.getHeight(), newStoredBlock.getHeader().getHash());
+                    return;
+                }
+                int splitPointHeight = splitPoint != null ? splitPoint.getHeight() : -1;
                 String splitPointHash =
                         splitPoint != null ? splitPoint.getHeader().getHashAsString() : "?";
-                log.info("Block forks the chain at {}, but it did not cause a reorganize:\n{}",
+                log.info("Block forks the chain at height {}/block {}, but it did not cause a reorganize:\n{}",
-                        splitPointHash, newStoredBlock);
+                        new Object[]{splitPointHeight, splitPointHash, newStoredBlock});
             }
 
-            // We may not have any transactions if we received only a header. That never happens today but will in
+            // We may not have any transactions if we received only a header, which can happen during fast catchup.
-            // future when getheaders is used as an optimization.
+            // If we do, send them to the wallet but state that they are on a side chain so it knows not to try and
+            // spend them until they become activated.
             if (newTransactions != null) {
                 sendTransactionsToWallet(newStoredBlock, NewBlockType.SIDE_CHAIN, newTransactions);
             }
@@ -288,7 +300,8 @@ public class BlockChain {
 
     /**
      * Locates the point in the chain at which newStoredBlock and chainHead diverge. Returns null if no split point was
-     * found (ie they are part of the same chain).
+     * found (ie they are not part of the same chain). Returns newChainHead or chainHead if they don't actually diverge
+     * but are part of the same chain.
      */
     private StoredBlock findSplit(StoredBlock newChainHead, StoredBlock chainHead) throws BlockStoreException {
         StoredBlock currentChainCursor = chainHead;
@@ -359,10 +372,12 @@ public class BlockChain {
             Iterator<Block> iter = unconnectedBlocks.iterator();
             while (iter.hasNext()) {
                 Block block = iter.next();
+                log.debug("Trying to connect {}", block.getHash());
                 // Look up the blocks previous.
                 StoredBlock prev = blockStore.get(block.getPrevBlockHash());
                 if (prev == null) {
                     // This is still an unconnected/orphan block.
+                    log.debug("  but it is not connectable right now");
                     continue;
                 }
                 // Otherwise we can connect it now.

tests/com/google/bitcoin/core/BlockChainTest.java

@@ -18,6 +18,7 @@ package com.google.bitcoin.core;
 
 import com.google.bitcoin.store.BlockStore;
 import com.google.bitcoin.store.MemoryBlockStore;
+import com.google.bitcoin.utils.BriefLogFormatter;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -38,6 +39,7 @@ public class BlockChainTest {
     private BlockStore blockStore;
     private Address coinbaseTo;
     private NetworkParameters unitTestParams;
+    private final StoredBlock[] block = new StoredBlock[1];
 
     private void resetBlockStore() {
         blockStore = new MemoryBlockStore(unitTestParams);
@@ -45,9 +47,16 @@ public class BlockChainTest {
 
     @Before
     public void setUp() throws Exception {
+        BriefLogFormatter.initVerbose();
         testNetChain = new BlockChain(testNet, new Wallet(testNet), new MemoryBlockStore(testNet));
         unitTestParams = NetworkParameters.unitTests();
-        wallet = new Wallet(unitTestParams);
+        wallet = new Wallet(unitTestParams) {
+            @Override
+            public void receiveFromBlock(Transaction tx, StoredBlock block, BlockChain.NewBlockType blockType) throws VerificationException, ScriptException {
+                super.receiveFromBlock(tx, block, blockType);
+                BlockChainTest.this.block[0] = block;
+            }
+        };
         wallet.addKey(new ECKey());
 
         resetBlockStore();
@@ -120,7 +129,7 @@ public class BlockChainTest {
     }
 
     @Test
-    public void testUnconnectedBlocks() throws Exception {
+    public void unconnectedBlocks() throws Exception {
         Block b1 = unitTestParams.genesisBlock.createNextBlock(coinbaseTo);
         Block b2 = b1.createNextBlock(coinbaseTo);
         Block b3 = b2.createNextBlock(coinbaseTo);
@@ -135,7 +144,7 @@ public class BlockChainTest {
     }
 
     @Test
-    public void testDifficultyTransitions() throws Exception {
+    public void difficultyTransitions() throws Exception {
         // Add a bunch of blocks in a loop until we reach a difficulty transition point. The unit test params have an
         // artificially shortened period.
         Block prev = unitTestParams.genesisBlock;
@@ -163,7 +172,7 @@ public class BlockChainTest {
     }
 
     @Test
-    public void testBadDifficulty() throws Exception {
+    public void badDifficulty() throws Exception {
         assertTrue(testNetChain.add(getBlock1()));
         Block b2 = getBlock2();
         assertTrue(testNetChain.add(b2));
@@ -202,6 +211,25 @@ public class BlockChainTest {
         // TODO: Test difficulty change is not out of range when a transition period becomes valid.
     }
 
+    @Test
+    public void duplicates() throws Exception {
+        // Adding a block twice should not have any effect, in particular it should not send the block to the wallet.
+        Block b1 = unitTestParams.genesisBlock.createNextBlock(coinbaseTo);
+        Block b2 = b1.createNextBlock(coinbaseTo);
+        Block b3 = b2.createNextBlock(coinbaseTo);
+        assertTrue(chain.add(b1));
+        assertEquals(b1, block[0].getHeader());
+        assertTrue(chain.add(b2));
+        assertEquals(b2, block[0].getHeader());
+        assertTrue(chain.add(b3));
+        assertEquals(b3, block[0].getHeader());
+        assertEquals(b3, chain.getChainHead().getHeader());
+        assertTrue(chain.add(b2));
+        assertEquals(b3, chain.getChainHead().getHeader());
+        // Wallet was NOT called with the new block because the duplicate add was spotted.
+        assertEquals(b3, block[0].getHeader());
+    }
+
     // Some blocks from the test net.
     private Block getBlock2() throws Exception {
         Block b2 = new Block(testNet);