Fix a bug that could cause inconsistent wallets.

When a side chain block contains the same transactions as the best chain, we incorrectly inserted into the inactive map, triggering assertion failures. Resolves issue 202.

core/src/main/java/com/google/bitcoin/core/Block.java

@@ -725,13 +725,17 @@ public class Block extends Message {
         hash = null;
     }
 
-	/** Adds a transaction to this block. */
+	/** Adds a transaction to this block. The nonce and merkle root are invalid after this. */
-    void addTransaction(Transaction t) {
+    public void addTransaction(Transaction t) {
         unCacheTransactions();
         if (transactions == null) {
             transactions = new ArrayList<Transaction>();
         }
         t.setParent(this);
+        if (transactions.size() == 0 && !t.isCoinBase())
+            throw new RuntimeException("Attempted to add a non-coinbase transaction as the first transaction: " + t);
+        else if (transactions.size() > 0 && t.isCoinBase())
+            throw new RuntimeException("Attempted to add a coinbase transaction when there already is one: " + t);
         transactions.add(t);
         adjustLength(t.length);
         // Force a recalculation next time the values are needed.

core/src/main/java/com/google/bitcoin/core/Wallet.java

@@ -504,7 +504,7 @@ public class Wallet implements Serializable {
         BigInteger valueDifference = valueSentToMe.subtract(valueSentFromMe);
 
         if (!reorg) {
-            log.info("Received tx {} for {} BTC: {}", new Object[]{sideChain ? " on a side chain" : "",
+            log.info("Received tx {} for {} BTC: {}", new Object[]{sideChain ? "on a side chain" : "",
                     bitcoinValueToFriendlyString(valueDifference), tx.getHashAsString()});
         }
 
@@ -518,7 +518,6 @@ public class Wallet implements Serializable {
             log.info("  <-pending");
             // A transaction we created appeared in a block. Probably this is a spend we broadcast that has been
             // accepted by the network.
-            //
             if (bestChain) {
                 if (valueSentToMe.equals(BigInteger.ZERO)) {
                     // There were no change transactions so this tx is fully spent.
@@ -551,8 +550,13 @@ public class Wallet implements Serializable {
             // This TX didn't originate with us. It could be sending us coins and also spending our own coins if keys
             // are being shared between different wallets.
             if (sideChain) {
-                log.info("  ->inactive");
+                if (unspent.containsKey(tx.getHash()) || spent.containsKey(tx.getHash())) {
-                inactive.put(tx.getHash(), tx);
+                    // This side chain block contains transactions that already appeared in the best chain. It's normal,
+                    // we don't need to consider this transaction inactive, we can just ignore it.
+                } else {
+                    log.info("  ->inactive");
+                    inactive.put(tx.getHash(), tx);
+                }
             } else if (bestChain) {
                 // This can trigger tx confidence listeners to be run in the case of double spends. We may need to
                 // delay the execution of the listeners until the bottom to avoid the wallet mutating during updates.

core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java

@@ -56,7 +56,6 @@ public class ChainSplitTest {
     public void testForking1() throws Exception {
         // Check that if the block chain forks, we end up using the right chain. Only tests inbound transactions
         // (receiving coins). Checking that we understand reversed spends is in testForking2.
-
         final boolean[] reorgHappened = new boolean[1];
         reorgHappened[0] = false;
         wallet.addEventListener(new AbstractWalletEventListener() {
@@ -187,6 +186,25 @@ public class ChainSplitTest {
         assertEquals(Utils.toNanoCoins(0, 0), wallet.getBalance());
     }
 
+    @Test
+    public void testForking5() throws Exception {
+        // Test the standard case in which a block containing identical transactions appears on a side chain.
+        Block b1 = unitTestParams.genesisBlock.createNextBlock(coinsTo);
+        chain.add(b1);
+        assertEquals("50.00", Utils.bitcoinValueToFriendlyString(wallet.getBalance()));
+        // genesis -> b1
+        //         -> b2
+        Block b2 = unitTestParams.genesisBlock.createNextBlock(coinsTo);
+        Transaction b2coinbase = b2.transactions.get(0);
+        b2.transactions.clear();
+        b2.addTransaction(b2coinbase);
+        b2.addTransaction(b1.transactions.get(1));
+        b2.solve();
+        chain.add(b2);
+        assertEquals("50.00", Utils.bitcoinValueToFriendlyString(wallet.getBalance()));
+        assertTrue(wallet.isConsistent());
+    }
+
     @Test
     public void testDoubleSpendOnFork() throws Exception {
         // Check what happens when a re-org happens and one of our confirmed transactions becomes invalidated by a
@@ -232,7 +250,6 @@ public class ChainSplitTest {
     public void testDoubleSpendOnForkPending() throws Exception {
         // Check what happens when a re-org happens and one of our UNconfirmed transactions becomes invalidated by a
         // double spend on the new best chain.
-
         final Transaction[] eventDead = new Transaction[1];
         final Transaction[] eventReplacement = new Transaction[1];
         wallet.addEventListener(new AbstractWalletEventListener() {

core/src/test/java/com/google/bitcoin/core/WalletTest.java

@@ -16,7 +16,6 @@
 
 package com.google.bitcoin.core;
 
-import com.google.bitcoin.core.Wallet.BalanceType;
 import com.google.bitcoin.core.WalletTransaction.Pool;
 import com.google.bitcoin.store.BlockStore;
 import com.google.bitcoin.store.MemoryBlockStore;
@@ -269,7 +268,8 @@ public class WalletTest {
 
     @Test
     public void isConsistent_duplicates() throws Exception {
-        // This test ensures that isConsistent catches duplicate transactions.
+        // This test ensures that isConsistent catches duplicate transactions, eg, because we submitted the same block
+        // twice (this is not allowed).
         Transaction tx = createFakeTx(params, Utils.toNanoCoins(1, 0), myAddress);
         Address someOtherGuy = new ECKey().toAddress(params);
         TransactionOutput output = new TransactionOutput(params, tx, Utils.toNanoCoins(0, 5), someOtherGuy);
@@ -280,7 +280,7 @@ public class WalletTest {
         
         Transaction txClone = new Transaction(params, tx.bitcoinSerialize());
         try {
-            wallet.receiveFromBlock(txClone, null, BlockChain.NewBlockType.SIDE_CHAIN);
+            wallet.receiveFromBlock(txClone, null, BlockChain.NewBlockType.BEST_CHAIN);
             fail();
         } catch (IllegalStateException ex) {
             // expected