diff --git a/src/com/google/bitcoin/core/BlockChain.java b/src/com/google/bitcoin/core/BlockChain.java index 058a39e7..8cdbf9e4 100644 --- a/src/com/google/bitcoin/core/BlockChain.java +++ b/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{}", - splitPointHash, newStoredBlock); + log.info("Block forks the chain at height {}/block {}, but it did not cause a reorganize:\n{}", + new Object[]{splitPointHeight, splitPointHash, newStoredBlock}); } - // We may not have any transactions if we received only a header. That never happens today but will in - // future when getheaders is used as an optimization. + // We may not have any transactions if we received only a header, which can happen during fast catchup. + // 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 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. diff --git a/tests/com/google/bitcoin/core/BlockChainTest.java b/tests/com/google/bitcoin/core/BlockChainTest.java index eea56d53..187041d3 100644 --- a/tests/com/google/bitcoin/core/BlockChainTest.java +++ b/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);