From d6e6f397738bc0f9f489640175067439120a9cf9 Mon Sep 17 00:00:00 2001 From: sqrrm Date: Mon, 30 Aug 2021 18:11:52 +0200 Subject: [PATCH 1/2] 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. --- .../main/java/org/bitcoinj/wallet/Wallet.java | 18 ++++- .../java/org/bitcoinj/wallet/WalletTest.java | 68 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/bitcoinj/wallet/Wallet.java b/core/src/main/java/org/bitcoinj/wallet/Wallet.java index a7cd09f05b6..299d305cf38 100644 --- a/core/src/main/java/org/bitcoinj/wallet/Wallet.java +++ b/core/src/main/java/org/bitcoinj/wallet/Wallet.java @@ -1891,7 +1891,14 @@ public void receivePending(Transaction tx, @Nullable List 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 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(); } diff --git a/core/src/test/java/org/bitcoinj/wallet/WalletTest.java b/core/src/test/java/org/bitcoinj/wallet/WalletTest.java index 872fbdfe33d..5d7e5efd743 100644 --- a/core/src/test/java/org/bitcoinj/wallet/WalletTest.java +++ b/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); + } } From 8bfa8c3e3f4da52cbeebe66e59dc9118fe665bd8 Mon Sep 17 00:00:00 2001 From: Sean Gilligan Date: Sat, 22 Aug 2020 10:26:00 -0700 Subject: [PATCH 2/2] SPVBlockStoreTest: Disable testing SPVBlockStore deletion on Windows. See issue #2032. --- .../org/bitcoinj/store/SPVBlockStoreTest.java | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/core/src/test/java/org/bitcoinj/store/SPVBlockStoreTest.java b/core/src/test/java/org/bitcoinj/store/SPVBlockStoreTest.java index 0daaaf5b0ce..bcf1dfedcaf 100644 --- a/core/src/test/java/org/bitcoinj/store/SPVBlockStoreTest.java +++ b/core/src/test/java/org/bitcoinj/store/SPVBlockStoreTest.java @@ -26,14 +26,7 @@ import java.util.Collections; import java.util.concurrent.TimeUnit; -import org.bitcoinj.core.Address; -import org.bitcoinj.core.Block; -import org.bitcoinj.core.ECKey; -import org.bitcoinj.core.LegacyAddress; -import org.bitcoinj.core.NetworkParameters; -import org.bitcoinj.core.Sha256Hash; -import org.bitcoinj.core.StoredBlock; -import org.bitcoinj.core.Transaction; +import org.bitcoinj.core.*; import org.bitcoinj.params.UnitTestParams; import org.junit.Before; import org.junit.Test; @@ -150,11 +143,13 @@ public void performanceTest() throws BlockStoreException { @Test public void oneStoreDelete() throws Exception { - // Used to fail on windows - // See https://github.com/bitcoinj/bitcoinj/issues/1477#issuecomment-450274821 SPVBlockStore store = new SPVBlockStore(UNITTEST, blockStoreFile); store.close(); - assertTrue(blockStoreFile.delete()); + boolean deleted = blockStoreFile.delete(); + if (!Utils.isWindows()) { + // TODO: Deletion is failing on Windows + assertTrue(deleted); + } } public void clear() throws Exception {