Wallet: clone transaction before committing

Problem: A transaction received from the network is added to all wallets
that find it relevant. If two wallets find the same transaction relevant
the same Transaction instance is added to both wallets.

Spending the outputs from this transaction can cause consistiency
issues, in particular if the outputs are spent in the same transaction,
as shown in WalletTest.oneTxTwoWallets. There are probably more issues
with having the same Transaction instance handled by two different
wallets.

Fix: Clone the transaction before adding it to the wallet.

core/src/main/java/org/bitcoinj/wallet/Wallet.java

@@ -1891,7 +1891,14 @@ public void receivePending(Transaction tx, @Nullable List<Transaction> dependenc
                 return;
             if (isTransactionRisky(tx, dependencies) && !acceptRiskyTransactions) {
                 // isTransactionRisky already logged the reason.
-                riskDropped.put(tx.getTxId(), tx);
+
+                // Clone transaction to avoid multiple wallets pointing to the same transaction. This can happen when
+                // two wallets depend on the same transaction.
+                Transaction cloneTx = tx.getParams().getDefaultSerializer().makeTransaction(tx.bitcoinSerialize());
+                cloneTx.setPurpose(tx.getPurpose());
+                cloneTx.setUpdateTime(tx.getUpdateTime());
+
+                riskDropped.put(cloneTx.getTxId(), cloneTx);
                 log.warn("There are now {} risk dropped transactions being kept in memory", riskDropped.size());
                 return;
             }
@@ -1903,10 +1910,17 @@ public void receivePending(Transaction tx, @Nullable List<Transaction> dependenc
             if (tx.getConfidence().getSource().equals(TransactionConfidence.Source.UNKNOWN)) {
                 log.warn("Wallet received transaction with an unknown source. Consider tagging it!");
             }
+
+            // Clone transaction to avoid multiple wallets pointing to the same transaction. This can happen when
+            // two wallets depend on the same transaction.
+            Transaction cloneTx = tx.getParams().getDefaultSerializer().makeTransaction(tx.bitcoinSerialize());
+            cloneTx.setPurpose(tx.getPurpose());
+            cloneTx.setUpdateTime(tx.getUpdateTime());
+
             // If this tx spends any of our unspent outputs, mark them as spent now, then add to the pending pool. This
             // ensures that if some other client that has our keys broadcasts a spend we stay in sync. Also updates the
             // timestamp on the transaction and registers/runs event listeners.
-            commitTx(tx);
+            commitTx(cloneTx);
         } finally {
             lock.unlock();
         }

core/src/test/java/org/bitcoinj/wallet/WalletTest.java

@@ -35,6 +35,7 @@
 import org.bitcoinj.core.TransactionInput;
 import org.bitcoinj.core.TransactionOutPoint;
 import org.bitcoinj.core.TransactionOutput;
+import org.bitcoinj.core.TransactionWitness;
 import org.bitcoinj.core.Utils;
 import org.bitcoinj.core.VerificationException;
 import org.bitcoinj.core.TransactionConfidence.ConfidenceType;
@@ -3735,4 +3736,71 @@ public void roundtripViaMnemonicCode() {
         assertEquals(wallet.freshReceiveAddress(Script.ScriptType.P2PKH),
                 clone.freshReceiveAddress(Script.ScriptType.P2PKH));
     }
+
+    @Test
+    public void oneTxTwoWallets() {
+        Wallet wallet1 = Wallet.createDeterministic(UNITTEST, Script.ScriptType.P2WPKH);
+        Wallet wallet2 = Wallet.createDeterministic(UNITTEST, Script.ScriptType.P2WPKH);
+        Address address1 = wallet1.freshReceiveAddress(Script.ScriptType.P2PKH);
+        Address address2 = wallet2.freshReceiveAddress(Script.ScriptType.P2PKH);
+
+        // Both wallet1 and wallet2 receive coins in the same tx
+        Transaction tx0 = createFakeTx(UNITTEST);
+        Transaction tx1 = new Transaction(UNITTEST);
+        tx1.addInput(tx0.getOutput(0));
+        tx1.addOutput(COIN, address1); // to wallet1
+        tx1.addOutput(COIN, address2); // to wallet2
+        tx1.addOutput(COIN, OTHER_ADDRESS);
+        wallet1.receivePending(tx1, null);
+        wallet2.receivePending(tx1, null);
+
+        // Confirm transactions in both wallets
+        StoredBlock block = createFakeBlock(blockStore, Block.BLOCK_HEIGHT_GENESIS, tx1).storedBlock;
+        wallet1.notifyTransactionIsInBlock(tx1.getTxId(), block, AbstractBlockChain.NewBlockType.BEST_CHAIN, 1);
+        wallet2.notifyTransactionIsInBlock(tx1.getTxId(), block, AbstractBlockChain.NewBlockType.BEST_CHAIN, 1);
+
+        assertEquals(COIN, wallet1.getTotalReceived());
+        assertEquals(COIN, wallet2.getTotalReceived());
+
+        // Spend two outputs from the same tx from two different wallets
+        SendRequest sendReq = SendRequest.to(OTHER_ADDRESS, valueOf(2, 0));
+        sendReq.tx.addInput(tx1.getOutput(0));
+        sendReq.tx.addInput(tx1.getOutput(1));
+
+        // Wallet1 sign input 0
+        TransactionInput inputW1 = sendReq.tx.getInput(0);
+        ECKey sigKey1 = inputW1.getOutpoint().getConnectedKey(wallet1);
+        Script scriptCode1 = ScriptBuilder.createP2PKHOutputScript(sigKey1);
+        TransactionSignature txSig1 = sendReq.tx.calculateWitnessSignature(0, sigKey1, scriptCode1,
+                inputW1.getValue(), Transaction.SigHash.ALL, false);
+        inputW1.setScriptSig(ScriptBuilder.createEmpty());
+        inputW1.setWitness(TransactionWitness.redeemP2WPKH(txSig1, sigKey1));
+
+        // Wallet2 sign input 1
+        TransactionInput inputW2 = sendReq.tx.getInput(1);
+        ECKey sigKey2 = inputW2.getOutpoint().getConnectedKey(wallet2);
+        Script scriptCode2 = ScriptBuilder.createP2PKHOutputScript(sigKey2);
+        TransactionSignature txSig2 = sendReq.tx.calculateWitnessSignature(0, sigKey2, scriptCode2,
+                inputW2.getValue(), Transaction.SigHash.ALL, false);
+        inputW2.setScriptSig(ScriptBuilder.createEmpty());
+        inputW2.setWitness(TransactionWitness.redeemP2WPKH(txSig2, sigKey2));
+
+        wallet1.commitTx(sendReq.tx);
+        wallet2.commitTx(sendReq.tx);
+        assertEquals(ZERO, wallet1.getBalance());
+        assertEquals(ZERO, wallet2.getBalance());
+
+        assertTrue(wallet1.isConsistent());
+        assertTrue(wallet2.isConsistent());
+
+        Transaction txW1 = wallet1.getTransaction(tx1.getTxId());
+        Transaction txW2 = wallet2.getTransaction(tx1.getTxId());
+
+        assertEquals(txW1, tx1);
+        assertNotSame(txW1, tx1);
+        assertEquals(txW2, tx1);
+        assertNotSame(txW2, tx1);
+        assertEquals(txW1, txW2);
+        assertNotSame(txW1, txW2);
+    }
 }