Set outpoint.fromTx during TransactionInput.connect even in the conflict case. Resolves issue 181. Also introduce a helper for creating double spends and rewrite the test case for this in WalletProtobufSerializer to cover more codepaths. Add a comment noting that in the double spending case the overriding transaction isn't presently being stored in the wallet.

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

@@ -24,6 +24,8 @@ import java.io.OutputStream;
 import java.io.Serializable;
 import java.util.Map;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 /**
  * A transfer of coins from one address to another creates a transaction in which the outputs
  * can be claimed by the recipient in the input of another transaction. You can imagine a
@@ -104,16 +106,15 @@ public class TransactionInput extends ChildMessage implements Serializable {
      * @param params NetworkParameters object.
      * @param msg Bitcoin protocol formatted byte array containing message content.
      * @param offset The location of the first msg byte within the array.
-     * @param protocolVersion Bitcoin protocol version.
      * @param parseLazy Whether to perform a full parse immediately or delay until a read is requested.
      * @param parseRetain Whether to retain the backing byte array for quick reserialization.  
      * If true and the backing byte array is invalidated due to modification of a field then 
      * the cached bytes may be repopulated and retained if the message is serialized again in the future.
-     * @param length The length of message if known.  Usually this is provided when deserializing of the wire
      * as the length will be provided as part of the header.  If unknown then set to Message.UNKNOWN_LENGTH
      * @throws ProtocolException
      */
-    public TransactionInput(NetworkParameters params, Transaction parentTransaction, byte[] msg, int offset, boolean parseLazy, boolean parseRetain)
+    public TransactionInput(NetworkParameters params, Transaction parentTransaction, byte[] msg, int offset,
+                            boolean parseLazy, boolean parseRetain)
             throws ProtocolException {
         super(params, msg, offset, parentTransaction, parseLazy, parseRetain, UNKNOWN_LENGTH);
         this.parentTransaction = parentTransaction;
@@ -274,31 +275,37 @@ public class TransactionInput extends ChildMessage implements Serializable {
 
     /**
      * Connects this input to the relevant output of the referenced transaction if it's in the given map.
-     * Connecting means updating the internal pointers and spent flags.
+     * Connecting means updating the internal pointers and spent flags. If the mode is to ABORT_ON_CONFLICT then
-     *
+     * the spent output won't be changed, but the outpoint.fromTx pointer will still be updated.
      *
      * @param transactions Map of txhash->transaction.
-     * @param disconnect   Whether to abort if there's a pre-existing connection or not.
+     * @param mode   Whether to abort if there's a pre-existing connection or not.
      * @return true if connection took place, false if the referenced transaction was not in the list.
      */
-    ConnectionResult connect(Map<Sha256Hash, Transaction> transactions, ConnectMode disconnect) {
+    ConnectionResult connect(Map<Sha256Hash, Transaction> transactions, ConnectMode mode) {
         Transaction tx = transactions.get(outpoint.getHash());
-        if (tx == null)
+        if (tx == null) {
             return TransactionInput.ConnectionResult.NO_SUCH_TX;
+        }
         TransactionOutput out = tx.getOutputs().get((int) outpoint.getIndex());
         if (!out.isAvailableForSpending()) {
-            if (disconnect == ConnectMode.DISCONNECT_ON_CONFLICT)
+            if (mode == ConnectMode.DISCONNECT_ON_CONFLICT) {
                 out.markAsUnspent();
-            else if (disconnect == ConnectMode.ABORT_ON_CONFLICT)
+            } else if (mode == ConnectMode.ABORT_ON_CONFLICT) {
+                outpoint.fromTx = checkNotNull(out.parentTransaction);
                 return TransactionInput.ConnectionResult.ALREADY_SPENT;
-            else
+            }
-                throw new UnsupportedOperationException();  // Unreachable.
         }
-        outpoint.fromTx = tx;
+        connect(out);
-        out.markAsSpent(this);
         return TransactionInput.ConnectionResult.SUCCESS;
     }
 
+    /** Internal use only: connects this TransactionInput to the given output (updates pointers and spent flags) */
+    public void connect(TransactionOutput out) {
+        outpoint.fromTx = checkNotNull(out.parentTransaction);
+        out.markAsSpent(this);
+    }
+
     /**
      * Release the connected output, making it spendable once again.
      *

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

@@ -610,10 +610,8 @@ public class Wallet implements Serializable {
                 checkNotNull(doubleSpent);
                 int index = (int) input.getOutpoint().getIndex();
                 TransactionOutput output = doubleSpent.getOutputs().get(index);
-                TransactionInput spentBy = output.getSpentBy();
+                TransactionInput spentBy = checkNotNull(output.getSpentBy());
-                checkNotNull(spentBy);
+                Transaction connected = checkNotNull(spentBy.getParentTransaction());
-                Transaction connected = spentBy.getParentTransaction();
-                checkNotNull(connected);
                 if (fromChain) {
                     // This must have overridden a pending tx, or the block is bad (contains transactions
                     // that illegally double spend: should never occur if we are connected to an honest node).
@@ -641,7 +639,7 @@ public class Wallet implements Serializable {
                 // Otherwise we saw a transaction spend our coins, but we didn't try and spend them ourselves yet.
                 // The outputs are already marked as spent by the connect call above, so check if there are any more for
                 // us to use. Move if not.
-                Transaction connected = input.getOutpoint().fromTx;
+                Transaction connected = checkNotNull(input.getOutpoint().fromTx);
                 maybeMoveTxToSpent(connected, "prevtx");
             }
         }

core/src/main/java/com/google/bitcoin/store/WalletProtobufSerializer.java

@@ -300,7 +300,8 @@ public class WalletProtobufSerializer {
             if (transactionOutput.hasSpentByTransactionHash()) {
                 Transaction spendingTx = txMap.get(transactionOutput.getSpentByTransactionHash());
                 final int spendingIndex = transactionOutput.getSpentByTransactionIndex();
-                output.markAsSpent(spendingTx.getInputs().get(spendingIndex));
+                TransactionInput input = spendingTx.getInputs().get(spendingIndex);
+                input.connect(output);
             }
         }
         

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

@@ -41,6 +41,42 @@ public class TestUtils {
         return t;
     }
 
+    public static class DoubleSpends {
+        public Transaction t1, t2, prevTx;
+    }
+
+    /**
+     * Creates two transactions that spend the same (fake) output. t1 spends to "to". t2 spends somewhere else.
+     * The fake output goes to the same address as t2.
+     */
+    public static DoubleSpends createFakeDoubleSpendTxns(NetworkParameters params, Address to) {
+        DoubleSpends doubleSpends = new DoubleSpends();
+        BigInteger value = Utils.toNanoCoins(1, 0);
+        Address someBadGuy = new ECKey().toAddress(params);
+
+        doubleSpends.t1 = new Transaction(params);
+        TransactionOutput o1 = new TransactionOutput(params, doubleSpends.t1, value, to);
+        doubleSpends.t1.addOutput(o1);
+
+        doubleSpends.prevTx = new Transaction(params);
+        TransactionOutput prevOut = new TransactionOutput(params, doubleSpends.prevTx, value, someBadGuy);
+        doubleSpends.prevTx.addOutput(prevOut);
+        doubleSpends.t1.addInput(prevOut);
+
+        doubleSpends.t2 = new Transaction(params);
+        doubleSpends.t2.addInput(prevOut);
+        TransactionOutput o2 = new TransactionOutput(params, doubleSpends.t2, value, someBadGuy);
+        doubleSpends.t2.addOutput(o2);
+
+        try {
+            doubleSpends.t1 = new Transaction(params, doubleSpends.t1.bitcoinSerialize());
+            doubleSpends.t2 = new Transaction(params, doubleSpends.t2.bitcoinSerialize());
+        } catch (ProtocolException e) {
+            throw new RuntimeException(e);
+        }
+        return doubleSpends;
+    }
+
     public static class BlockPair {
         StoredBlock storedBlock;
         Block block;

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

@@ -309,7 +309,7 @@ public class WalletTest {
     }
 
     @Test
-    public void finneyAttack() throws Exception {
+    public void doubleSpendFinneyAttack() throws Exception {
         // A Finney attack is where a miner includes a transaction spending coins to themselves but does not
         // broadcast it. When they find a solved block, they hold it back temporarily whilst they buy something with
         // those same coins. After purchasing, they broadcast the block thus reversing the transaction. It can be
@@ -340,6 +340,7 @@ public class WalletTest {
         Transaction send1 = wallet.createSend(new ECKey().toAddress(params), toNanoCoins(0, 50));
         // Create a double spend.
         Transaction send2 = wallet.createSend(new ECKey().toAddress(params), toNanoCoins(0, 50));
+        send2 = new Transaction(params, send2.bitcoinSerialize());
         // Broadcast send1.
         wallet.commitTx(send1);
         // Receive a block that overrides it.
@@ -352,27 +353,15 @@ public class WalletTest {
         // Receive 10 BTC.
         nanos = Utils.toNanoCoins(10, 0);
 
-        // Create a double spending tx.
+        TestUtils.DoubleSpends doubleSpends = TestUtils.createFakeDoubleSpendTxns(params, myAddress);
-        Transaction t2 = new Transaction(params);
+        // t1 spends to our wallet. t2 double spends somewhere else.
-        TransactionOutput o1 = new TransactionOutput(params, t2, nanos, myAddress);
+        wallet.receivePending(doubleSpends.t1);
-        t2.addOutput(o1);
+        assertEquals(TransactionConfidence.ConfidenceType.NOT_SEEN_IN_CHAIN,
-        Transaction prevTx = new Transaction(params);
+                     doubleSpends.t1.getConfidence().getConfidenceType());
-        Address someBadGuy = new ECKey().toAddress(params);
+        wallet.receiveFromBlock(doubleSpends.t2, null, BlockChain.NewBlockType.BEST_CHAIN);
-        TransactionOutput prevOut = new TransactionOutput(params, prevTx, nanos, someBadGuy);
-        prevTx.addOutput(prevOut);
-        // Connect it.
-        t2.addInput(prevOut);
-        wallet.receivePending(t2);
-        assertEquals(TransactionConfidence.ConfidenceType.NOT_SEEN_IN_CHAIN, t2.getConfidence().getConfidenceType());
-        // Receive a tx from a block that overrides it.
-        Transaction t3 = new Transaction(params);
-        TransactionOutput o3 = new TransactionOutput(params, t3, nanos, someBadGuy);
-        t3.addOutput(o3);
-        t3.addInput(prevOut);
-        wallet.receiveFromBlock(t3, null, BlockChain.NewBlockType.BEST_CHAIN);
         assertEquals(TransactionConfidence.ConfidenceType.OVERRIDDEN_BY_DOUBLE_SPEND, 
-                     t2.getConfidence().getConfidenceType());
+                     doubleSpends.t1.getConfidence().getConfidenceType());
-        assertEquals(t3, t2.getConfidence().getOverridingTransaction());
+        assertEquals(doubleSpends.t2, doubleSpends.t1.getConfidence().getOverridingTransaction());
     }
 
     @Test

core/src/test/java/com/google/bitcoin/store/WalletProtobufSerializerTest.java

@@ -3,6 +3,7 @@ package com.google.bitcoin.store;
 
 import com.google.bitcoin.core.*;
 import com.google.bitcoin.core.TransactionConfidence.ConfidenceType;
+import com.google.bitcoin.utils.BriefLogFormatter;
 import org.bitcoinj.wallet.Protos;
 import org.junit.Before;
 import org.junit.Test;
@@ -13,7 +14,6 @@ import java.io.IOException;
 import java.math.BigInteger;
 
 import static com.google.bitcoin.core.TestUtils.createFakeTx;
-import static com.google.bitcoin.core.Utils.toNanoCoins;
 import static org.junit.Assert.*;
 
 public class WalletProtobufSerializerTest {
@@ -24,6 +24,7 @@ public class WalletProtobufSerializerTest {
 
     @Before
     public void setUp() throws Exception {
+        BriefLogFormatter.initVerbose();
         myKey = new ECKey();
         myKey.setCreationTimeSeconds(123456789L);
         myAddress = myKey.toAddress(params);
@@ -32,23 +33,27 @@ public class WalletProtobufSerializerTest {
     }
 
     @Test
-    public void testSimple() throws Exception {
+    public void empty() throws Exception {
+        // Check the base case of a wallet with one key and no transactions.
         Wallet wallet1 = roundTrip(wallet);
         assertEquals(0, wallet1.getTransactions(true, true).size());
         assertEquals(BigInteger.ZERO, wallet1.getBalance());
-        
-        BigInteger v1 = Utils.toNanoCoins(1, 0);
-        Transaction t1 = createFakeTx(params, v1, myAddress);
-
-        wallet.receiveFromBlock(t1, null, BlockChain.NewBlockType.BEST_CHAIN);
-        
-        wallet1 = roundTrip(wallet);
         assertArrayEquals(myKey.getPubKey(),
                 wallet1.findKeyFromPubHash(myKey.getPubKeyHash()).getPubKey());
         assertArrayEquals(myKey.getPrivKeyBytes(),
                 wallet1.findKeyFromPubHash(myKey.getPubKeyHash()).getPrivKeyBytes());
         assertEquals(myKey.getCreationTimeSeconds(),
                 wallet1.findKeyFromPubHash(myKey.getPubKeyHash()).getCreationTimeSeconds());
+    }
+
+    @Test
+    public void oneTx() throws Exception {
+        // Check basic tx serialization.
+        BigInteger v1 = Utils.toNanoCoins(1, 0);
+        Transaction t1 = createFakeTx(params, v1, myAddress);
+
+        wallet.receiveFromBlock(t1, null, BlockChain.NewBlockType.BEST_CHAIN);
+        Wallet wallet1 = roundTrip(wallet);
         assertEquals(1, wallet1.getTransactions(true, true).size());
         assertEquals(v1, wallet1.getBalance());
         assertArrayEquals(t1.bitcoinSerialize(),
@@ -66,30 +71,28 @@ public class WalletProtobufSerializerTest {
         assertEquals(Protos.Transaction.Pool.UNSPENT, t1p.getPool());
         assertFalse(t1p.hasLockTime());
         assertFalse(t1p.getTransactionInput(0).hasSequence());
-        assertArrayEquals(t1.getInputs().get(0).getOutpoint().getHash().getBytes(), t1p.getTransactionInput(0).getTransactionOutPointHash().toByteArray());
+        assertArrayEquals(t1.getInputs().get(0).getOutpoint().getHash().getBytes(),
+                t1p.getTransactionInput(0).getTransactionOutPointHash().toByteArray());
         assertEquals(0, t1p.getTransactionInput(0).getTransactionOutPointIndex());
         assertEquals(t1p.getTransactionOutput(0).getValue(), v1.longValue());
-        
+    }
-        ECKey k2 = new ECKey();
-        BigInteger v2 = toNanoCoins(0, 50);
-        Transaction t2 = wallet.sendCoinsOffline(k2.toAddress(params), v2);
-        t2.getConfidence().setConfidenceType(ConfidenceType.OVERRIDDEN_BY_DOUBLE_SPEND);
-        t2.getConfidence().setOverridingTransaction(t1);
-        t1.getConfidence().setConfidenceType(ConfidenceType.BUILDING);
-        t1.getConfidence().setAppearedAtChainHeight(123);
-        wallet1 = roundTrip(wallet);
-        Transaction t1r = wallet1.getTransaction(t1.getHash());
-        Transaction t2r = wallet1.getTransaction(t2.getHash());
-        assertArrayEquals(t2.bitcoinSerialize(), t2r.bitcoinSerialize());
-        assertArrayEquals(t1.bitcoinSerialize(), t1r.bitcoinSerialize());
-        assertEquals(t1r.getOutputs().get(0).getSpentBy(), t2r.getInputs().get(0));
-        assertEquals(ConfidenceType.OVERRIDDEN_BY_DOUBLE_SPEND, t2r.getConfidence().getConfidenceType());
-        assertEquals(t1r, t2r.getConfidence().getOverridingTransaction());
-        assertEquals(ConfidenceType.BUILDING, t1r.getConfidence().getConfidenceType());
-        assertEquals(123, t1r.getConfidence().getAppearedAtChainHeight());
 
-        assertEquals(1, wallet1.getPendingTransactions().size());
+    @Test
-        assertEquals(2, wallet1.getTransactions(true, true).size());
+    public void doubleSpend() throws Exception {
+        // Check that we can serialize double spends correctly, as this is a slightly tricky case.
+        TestUtils.DoubleSpends doubleSpends = TestUtils.createFakeDoubleSpendTxns(params, myAddress);
+        // t1 spends to our wallet.
+        wallet.receivePending(doubleSpends.t1);
+        // t2 rolls back t1 and spends somewhere else.
+        wallet.receiveFromBlock(doubleSpends.t2, null, BlockChain.NewBlockType.BEST_CHAIN);
+        Wallet wallet1 = roundTrip(wallet);
+        assertEquals(1, wallet1.getTransactions(true, true).size());
+        Transaction t1 = wallet1.getTransaction(doubleSpends.t1.getHash());
+        assertEquals(ConfidenceType.OVERRIDDEN_BY_DOUBLE_SPEND, t1.getConfidence().getConfidenceType());
+        assertEquals(BigInteger.ZERO, wallet1.getBalance());
+
+        // TODO: Wallet should store overriding transactions even if they are not wallet-relevant.
+        // assertEquals(doubleSpends.t2, t1.getConfidence().getOverridingTransaction());
     }
     
     @Test