Fix a bug that could cause transactions to not move from the unspent to spent maps (efficiency issue but not a correctness issue). Add change outputs to the fake transactions created by TestUtils to reveal the issue and add more pool size tests. Resolves issue 89.

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

@@ -256,7 +256,7 @@ public class Transaction extends ChildMessage implements Serializable {
     }
 
     /**
-     * @return true if every output is marked as spent.
+     * Returns true if every output is marked as spent.
      */
     public boolean isEveryOutputSpent() {
         maybeParse();
@@ -267,6 +267,18 @@ public class Transaction extends ChildMessage implements Serializable {
         return true;
     }
 
+    /**
+     * Returns true if every output owned by the given wallet is spent.
+     */
+    public boolean isEveryOwnedOutputSpent(Wallet wallet) {
+        maybeParse();
+        for (TransactionOutput output : outputs) {
+            if (output.isAvailableForSpending() && output.isMine(wallet))
+                return false;
+        }
+        return true;
+    }
+
     /**
      * Returns the earliest time at which the transaction was seen (broadcast or included into the chain),
      * or null if that information isn't available.

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

@@ -390,7 +390,7 @@ public class Wallet implements Serializable {
      * If the transactions outputs are all marked as spent, and it's in the unspent map, move it.
      */
     private void maybeMoveTxToSpent(Transaction tx, String context) {
-        if (tx.isEveryOutputSpent()) {
+        if (tx.isEveryOwnedOutputSpent(this)) {
             // There's nothing left I can spend in this transaction.
             if (unspent.remove(tx.getHash()) != null) {
                 if (log.isInfoEnabled()) {

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

@@ -23,9 +23,14 @@ import java.math.BigInteger;
 
 public class TestUtils {
     public static Transaction createFakeTx(NetworkParameters params, BigInteger nanocoins, Address to) {
+        // Create a fake TX of sufficient realism to exercise the unit tests. Two outputs, one to us, one to somewhere
+        // else to simulate change.
         Transaction t = new Transaction(params);
-        TransactionOutput o1 = new TransactionOutput(params, t, nanocoins, to);
+        TransactionOutput outputToMe = new TransactionOutput(params, t, nanocoins, to);
-        t.addOutput(o1);
+        t.addOutput(outputToMe);
+        TransactionOutput change = new TransactionOutput(params, t, Utils.toNanoCoins(1, 11), 
+                new ECKey().toAddress(params));
+        t.addOutput(change);
         // Make a previous tx simply to send us sufficient coins. This prev tx is not really valid but it doesn't
         // matter for our purposes.
         Transaction prevTx = new Transaction(params);

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

@@ -26,10 +26,9 @@ import java.util.List;
 
 import static com.google.bitcoin.core.TestUtils.createFakeBlock;
 import static com.google.bitcoin.core.TestUtils.createFakeTx;
-import static com.google.bitcoin.core.Utils.*;
+import static com.google.bitcoin.core.Utils.bitcoinValueToFriendlyString;
-import static org.junit.Assert.assertEquals;
+import static com.google.bitcoin.core.Utils.toNanoCoins;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertTrue;
 
 public class WalletTest {
     static final NetworkParameters params = NetworkParameters.unitTests();
@@ -125,14 +124,20 @@ public class WalletTest {
         StoredBlock b1 = createFakeBlock(params, blockStore, t1).storedBlock;
         StoredBlock b2 = createFakeBlock(params, blockStore, t2).storedBlock;
         BigInteger expected = toNanoCoins(5, 50);
+        assertEquals(0, wallet.getPoolSize(Wallet.Pool.ALL));
         wallet.receive(t1, b1, BlockChain.NewBlockType.BEST_CHAIN);
+        assertEquals(1, wallet.getPoolSize(Wallet.Pool.UNSPENT));
         wallet.receive(t2, b2, BlockChain.NewBlockType.BEST_CHAIN);
+        assertEquals(2, wallet.getPoolSize(Wallet.Pool.UNSPENT));
         assertEquals(expected, wallet.getBalance());
 
         // Now spend one coin.
         BigInteger v3 = toNanoCoins(1, 0);
         Transaction spend = wallet.createSend(new ECKey().toAddress(params), v3);
         wallet.confirmSend(spend);
+        assertEquals(1, wallet.getPoolSize(Wallet.Pool.UNSPENT));
+        assertEquals(1, wallet.getPoolSize(Wallet.Pool.SPENT));
+        assertEquals(1, wallet.getPoolSize(Wallet.Pool.PENDING));
 
         // Available and estimated balances should not be the same. We don't check the exact available balance here
         // because it depends on the coin selection algorithm.
@@ -288,7 +293,7 @@ public class WalletTest {
         wallet.receive(tx2, b2, BlockChain.NewBlockType.BEST_CHAIN);
         // Check we got them back in order.
         List<Transaction> transactions = wallet.getTransactionsByTime();
-        assertEquals(tx2,  transactions.get(0));
+        assertEquals(tx2, transactions.get(0));
         assertEquals(tx1,  transactions.get(1));
         assertEquals(2, transactions.size());
         // Check we get only the last transaction if we request a subrage.
@@ -315,7 +320,7 @@ public class WalletTest {
         transactions = wallet.getTransactionsByTime();
         assertEquals(tx3,  transactions.get(0));
         assertEquals(tx2,  transactions.get(1));
-        assertEquals(tx1,  transactions.get(2));
+        assertEquals(tx1, transactions.get(2));
         assertEquals(3, transactions.size());
     }
 }