From ce61bd211d1db9f7186e5e9fa880d44bad7b62da Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Thu, 20 Feb 2014 21:57:25 +0530 Subject: [PATCH] Wallet: recursively kill transactions that depend on dead transactions. Resolves issue 186. --- .../bitcoin/core/TransactionConfidence.java | 7 +- .../bitcoin/core/TransactionOutput.java | 3 +- .../java/com/google/bitcoin/core/Wallet.java | 60 +++++----- .../google/bitcoin/core/ChainSplitTest.java | 113 +++++++++++------- .../com/google/bitcoin/core/WalletTest.java | 46 +++++-- 5 files changed, 140 insertions(+), 89 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java b/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java index 65d69c38..30badd7b 100644 --- a/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java +++ b/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java @@ -244,10 +244,12 @@ public class TransactionConfidence implements Serializable { * transaction becomes available. */ public synchronized void setConfidenceType(ConfidenceType confidenceType) { - // Don't inform the event listeners if the confidence didn't really change. if (confidenceType == this.confidenceType) return; this.confidenceType = confidenceType; + if (confidenceType != ConfidenceType.DEAD) { + overridingTransaction = null; + } if (confidenceType == ConfidenceType.PENDING) { depth = 0; appearedAtChainHeight = -1; @@ -390,7 +392,8 @@ public class TransactionConfidence implements Serializable { /** * Called when the transaction becomes newly dead, that is, we learn that one of its inputs has already been spent * in such a way that the double-spending transaction takes precedence over this one. It will not become valid now - * unless there is a re-org. Automatically sets the confidence type to DEAD. + * unless there is a re-org. Automatically sets the confidence type to DEAD. The overriding transaction may not + * directly double spend this one, but could also have double spent a dependency of this tx. */ public synchronized void setOverridingTransaction(@Nullable Transaction overridingTransaction) { this.overridingTransaction = overridingTransaction; diff --git a/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java b/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java index df8c22ed..1db50d86 100644 --- a/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java +++ b/core/src/main/java/com/google/bitcoin/core/TransactionOutput.java @@ -52,7 +52,7 @@ public class TransactionOutput extends ChildMessage implements Serializable { // was owned by us and was sent to somebody else. If false and spentBy is set it means this output was owned by // us and used in one of our own transactions (eg, because it is a change output). private boolean availableForSpending; - private TransactionInput spentBy; + @Nullable private TransactionInput spentBy; // A reference to the transaction which holds this output. Transaction parentTransaction; @@ -311,6 +311,7 @@ public class TransactionOutput extends ChildMessage implements Serializable { /** * Returns the connected input. */ + @Nullable public TransactionInput getSpentBy() { return spentBy; } diff --git a/core/src/main/java/com/google/bitcoin/core/Wallet.java b/core/src/main/java/com/google/bitcoin/core/Wallet.java index 7dae5af4..e5b25de6 100644 --- a/core/src/main/java/com/google/bitcoin/core/Wallet.java +++ b/core/src/main/java/com/google/bitcoin/core/Wallet.java @@ -201,7 +201,8 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi transactions = new HashMap(); eventListeners = new CopyOnWriteArrayList>(); extensions = new HashMap(); - confidenceChanged = new HashMap(); + // Use a linked hash map to ensure ordering of event listeners is correct. + confidenceChanged = new LinkedHashMap(); createTransientState(); } @@ -987,9 +988,13 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi boolean isDeadCoinbase = tx.isCoinBase() && dead.containsKey(tx.getHash()); if (isDeadCoinbase) { // There is a dead coinbase tx being received on the best chain. A coinbase tx is made dead when it moves - // to a side chain but it can be switched back on a reorg and 'resurrected' back to spent or unspent. - // So take it out of the dead pool. - log.info(" coinbase tx {} <-dead: confidence {}", tx.getHashAsString(), + // to a side chain but it can be switched back on a reorg and resurrected back to spent or unspent. + // So take it out of the dead pool. Note that we don't resurrect dependent transactions here, even though + // we could. Bitcoin Core nodes on the network have deleted the dependent transactions from their mempools + // entirely by this point. We could and maybe should rebroadcast them so the network remembers and tries + // to confirm them again. But this is a deeply unusual edge case that due to the maturity rule should never + // happen in practice, thus for simplicities sake we ignore it here. + log.info(" coinbase tx <-dead: confidence {}", tx.getHashAsString(), tx.getConfidence().getConfidenceType().name()); dead.remove(tx.getHash()); } @@ -1103,25 +1108,18 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi } } - private void killCoinbase(Transaction coinbase) { - log.warn("Coinbase killed by re-org: {}", coinbase.getHashAsString()); - coinbase.getConfidence().setOverridingTransaction(null); - confidenceChanged.put(coinbase, TransactionConfidence.Listener.ChangeReason.TYPE); - final Sha256Hash hash = coinbase.getHash(); - pending.remove(hash); - unspent.remove(hash); - spent.remove(hash); - addWalletTransaction(Pool.DEAD, coinbase); - // TODO: Properly handle the recursive nature of killing transactions here. - } - - // Updates the wallet when a double spend occurs. overridingTx/overridingInput can be null for the case of coinbases - private void killTx(Transaction overridingTx, List killedTx) { - for (Transaction tx : killedTx) { - log.warn("Saw double spend from chain override pending tx {}", tx.getHashAsString()); - log.warn(" <-pending ->dead killed by {}", overridingTx.getHashAsString()); + // Updates the wallet when a double spend occurs. overridingTx can be null for the case of coinbases + private void killTx(@Nullable Transaction overridingTx, List killedTx) { + LinkedList work = new LinkedList(killedTx); + while (!work.isEmpty()) { + final Transaction tx = work.poll(); + log.warn("TX {} killed{}", tx.getHashAsString(), + overridingTx != null ? "by " + overridingTx.getHashAsString() : ""); log.warn("Disconnecting each input and moving connected transactions."); + // TX could be pending (finney attack), or in unspent/spent (coinbase killed by reorg). pending.remove(tx.getHash()); + unspent.remove(tx.getHash()); + spent.remove(tx.getHash()); addWalletTransaction(Pool.DEAD, tx); for (TransactionInput deadInput : tx.getInputs()) { Transaction connected = deadInput.getOutpoint().fromTx; @@ -1131,7 +1129,17 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi } tx.getConfidence().setOverridingTransaction(overridingTx); confidenceChanged.put(tx, TransactionConfidence.Listener.ChangeReason.TYPE); + // Now kill any transactions we have that depended on this one. + for (TransactionOutput deadOutput : tx.getOutputs()) { + TransactionInput connected = deadOutput.getSpentBy(); + if (connected == null) continue; + final Transaction parentTransaction = connected.getParentTransaction(); + log.info("This death invalidated dependent tx {}", parentTransaction.getHash()); + work.push(parentTransaction); + } } + if (overridingTx == null) + return; log.warn("Now attempting to connect the inputs of the overriding transaction."); for (TransactionInput input : overridingTx.getInputs()) { TransactionInput.ConnectionResult result = input.connect(unspent, TransactionInput.ConnectMode.DISCONNECT_ON_CONFLICT); @@ -1144,7 +1152,6 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi } } } - // TODO: Recursively kill other transactions that were double spent. } /** @@ -2491,17 +2498,14 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi // graph we can never reliably kill all transactions we might have that were rooted in // this coinbase tx. Some can just go pending forever, like the Satoshi client. However we // can do our best. - // - // TODO: Is it better to try and sometimes fail, or not try at all? - killCoinbase(tx); + log.warn("Coinbase killed by re-org: {}", tx.getHashAsString()); + killTx(null, ImmutableList.of(tx)); } else { for (TransactionOutput output : tx.getOutputs()) { TransactionInput input = output.getSpentBy(); if (input != null) input.disconnect(); } - for (TransactionInput input : tx.getInputs()) { - input.disconnect(); - } + tx.disconnectInputs(); oldChainTxns.add(tx); unspent.remove(txHash); spent.remove(txHash); diff --git a/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java b/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java index 00918797..89e8c897 100644 --- a/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java +++ b/core/src/test/java/com/google/bitcoin/core/ChainSplitTest.java @@ -572,22 +572,19 @@ public class ChainSplitTest { public void coinbaseDeath() throws Exception { // Check that a coinbase tx is marked as dead after a reorg rather than pending as normal non-double-spent // transactions would be. Also check that a dead coinbase on a sidechain is resurrected if the sidechain - // becomes the best chain once more. + // becomes the best chain once more. Finally, check that dependent transactions are killed recursively. final ArrayList txns = new ArrayList(3); wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onCoinsReceived(Wallet wallet, Transaction tx, BigInteger prevBalance, BigInteger newBalance) { txns.add(tx); } - }); + }, Threading.SAME_THREAD); - // Start by building three blocks on top of the genesis block. - // The first block contains a normal transaction that spends to coinTo. - // The second block contains a coinbase transaction that spends to coinTo2. - // The third block contains a normal transaction that spends to coinTo. - Block b1 = unitTestParams.getGenesisBlock().createNextBlock(coinsTo); - Block b2 = b1.createNextBlockWithCoinbase(wallet.getKeys().get(1).getPubKey()); - Block b3 = b2.createNextBlock(coinsTo); + Block b1 = unitTestParams.getGenesisBlock().createNextBlock(someOtherGuy); + final ECKey coinsTo2 = wallet.getKeys().get(1); + Block b2 = b1.createNextBlockWithCoinbase(coinsTo2.getPubKey()); + Block b3 = b2.createNextBlock(someOtherGuy); log.debug("Adding block b1"); assertTrue(chain.add(b1)); @@ -600,21 +597,38 @@ public class ChainSplitTest { // genesis -> b1 -> b2 -> b3 // - // Check we have seen the three transactions. - Threading.waitForUserCode(); - assertEquals(3, txns.size()); + // Check we have seen the coinbase. + assertEquals(1, txns.size()); // Check the coinbase transaction is building and in the unspent pool only. - assertEquals(ConfidenceType.BUILDING, txns.get(1).getConfidence().getConfidenceType()); - assertTrue(!wallet.pending.containsKey(txns.get(1).getHash())); - assertTrue(wallet.unspent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.spent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.dead.containsKey(txns.get(1).getHash())); + final Transaction coinbase = txns.get(0); + assertEquals(ConfidenceType.BUILDING, coinbase.getConfidence().getConfidenceType()); + assertTrue(!wallet.pending.containsKey(coinbase.getHash())); + assertTrue(wallet.unspent.containsKey(coinbase.getHash())); + assertTrue(!wallet.spent.containsKey(coinbase.getHash())); + assertTrue(!wallet.dead.containsKey(coinbase.getHash())); + + // Add blocks to b3 until we can spend the coinbase. + Block firstTip = b3; + for (int i = 0; i < unitTestParams.getSpendableCoinbaseDepth() - 2; i++) { + firstTip = firstTip.createNextBlock(someOtherGuy); + chain.add(firstTip); + } + // ... and spend. + Transaction fodder = wallet.createSend(new ECKey().toAddress(unitTestParams), Utils.toNanoCoins(50, 0)); + wallet.commitTx(fodder); + final AtomicBoolean fodderIsDead = new AtomicBoolean(false); + fodder.getConfidence().addEventListener(new TransactionConfidence.Listener() { + @Override + public void onConfidenceChanged(Transaction tx, ChangeReason reason) { + fodderIsDead.set(tx.getConfidence().getConfidenceType() == ConfidenceType.DEAD); + } + }, Threading.SAME_THREAD); // Fork like this: // - // genesis -> b1 -> b2 -> b3 - // \-> b4 -> b5 -> b6 + // genesis -> b1 -> b2 -> b3 -> [...] + // \-> b4 -> b5 -> b6 -> [...] // // The b4/ b5/ b6 is now the best chain Block b4 = b1.createNextBlock(someOtherGuy); @@ -627,57 +641,64 @@ public class ChainSplitTest { assertTrue(chain.add(b5)); log.debug("Adding block b6"); assertTrue(chain.add(b6)); - Threading.waitForUserCode(); - // Transaction 1 (in block b2) is now on a side chain and should have confidence type of dead and be in the dead pool only - assertEquals(TransactionConfidence.ConfidenceType.DEAD, txns.get(1).getConfidence().getConfidenceType()); - assertTrue(!wallet.pending.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.unspent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.spent.containsKey(txns.get(1).getHash())); - assertTrue(wallet.dead.containsKey(txns.get(1).getHash())); + Block secondTip = b6; + for (int i = 0; i < unitTestParams.getSpendableCoinbaseDepth() - 2; i++) { + secondTip = secondTip.createNextBlock(someOtherGuy); + chain.add(secondTip); + } + + // Transaction 1 (in block b2) is now on a side chain and should have confidence type of dead and be in + // the dead pool only. + assertEquals(TransactionConfidence.ConfidenceType.DEAD, coinbase.getConfidence().getConfidenceType()); + assertTrue(!wallet.pending.containsKey(coinbase.getHash())); + assertTrue(!wallet.unspent.containsKey(coinbase.getHash())); + assertTrue(!wallet.spent.containsKey(coinbase.getHash())); + assertTrue(wallet.dead.containsKey(coinbase.getHash())); + assertTrue(fodderIsDead.get()); // ... and back to the first chain. - Block b7 = b3.createNextBlock(coinsTo); - Block b8 = b7.createNextBlock(coinsTo); + Block b7 = firstTip.createNextBlock(someOtherGuy); + Block b8 = b7.createNextBlock(someOtherGuy); log.debug("Adding block b7"); assertTrue(chain.add(b7)); log.debug("Adding block b8"); assertTrue(chain.add(b8)); - Threading.waitForUserCode(); // - // genesis -> b1 -> b2 -> b3 -> b7 -> b8 - // \-> b4 -> b5 -> b6 + // genesis -> b1 -> b2 -> b3 -> [...] -> b7 -> b8 + // \-> b4 -> b5 -> b6 -> [...] // // The coinbase transaction should now have confidence type of building once more and in the unspent pool only. - assertEquals(TransactionConfidence.ConfidenceType.BUILDING, txns.get(1).getConfidence().getConfidenceType()); - assertTrue(!wallet.pending.containsKey(txns.get(1).getHash())); - assertTrue(wallet.unspent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.spent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.dead.containsKey(txns.get(1).getHash())); + assertEquals(TransactionConfidence.ConfidenceType.BUILDING, coinbase.getConfidence().getConfidenceType()); + assertTrue(!wallet.pending.containsKey(coinbase.getHash())); + assertTrue(wallet.unspent.containsKey(coinbase.getHash())); + assertTrue(!wallet.spent.containsKey(coinbase.getHash())); + assertTrue(!wallet.dead.containsKey(coinbase.getHash())); + // However, fodder is still dead. Bitcoin Core doesn't keep killed transactions around in case they become + // valid again later. They are just deleted from the mempool for good. // ... make the side chain dominant again. - Block b9 = b6.createNextBlock(coinsTo); - Block b10 = b9.createNextBlock(coinsTo); + Block b9 = secondTip.createNextBlock(someOtherGuy); + Block b10 = b9.createNextBlock(someOtherGuy); log.debug("Adding block b9"); assertTrue(chain.add(b9)); log.debug("Adding block b10"); assertTrue(chain.add(b10)); - Threading.waitForUserCode(); // - // genesis -> b1 -> b2 -> b3 -> b7 -> b8 - // \-> b4 -> b5 -> b6 -> b9 -> b10 + // genesis -> b1 -> b2 -> b3 -> [...] -> b7 -> b8 + // \-> b4 -> b5 -> b6 -> [...] -> b9 -> b10 // // The coinbase transaction should now have the confidence type of dead and be in the dead pool only. - assertEquals(TransactionConfidence.ConfidenceType.DEAD, txns.get(1).getConfidence().getConfidenceType()); - assertTrue(!wallet.pending.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.unspent.containsKey(txns.get(1).getHash())); - assertTrue(!wallet.spent.containsKey(txns.get(1).getHash())); - assertTrue(wallet.dead.containsKey(txns.get(1).getHash())); + assertEquals(TransactionConfidence.ConfidenceType.DEAD, coinbase.getConfidence().getConfidenceType()); + assertTrue(!wallet.pending.containsKey(coinbase.getHash())); + assertTrue(!wallet.unspent.containsKey(coinbase.getHash())); + assertTrue(!wallet.spent.containsKey(coinbase.getHash())); + assertTrue(wallet.dead.containsKey(coinbase.getHash())); } } diff --git a/core/src/test/java/com/google/bitcoin/core/WalletTest.java b/core/src/test/java/com/google/bitcoin/core/WalletTest.java index dd1b13fe..f23c5acd 100644 --- a/core/src/test/java/com/google/bitcoin/core/WalletTest.java +++ b/core/src/test/java/com/google/bitcoin/core/WalletTest.java @@ -20,8 +20,6 @@ import com.google.bitcoin.core.Transaction.SigHash; import com.google.bitcoin.core.Wallet.SendRequest; import com.google.bitcoin.wallet.DefaultCoinSelector; import com.google.bitcoin.wallet.RiskAnalysis; -import com.google.bitcoin.wallet.WalletTransaction; -import com.google.bitcoin.wallet.WalletTransaction.Pool; import com.google.bitcoin.crypto.KeyCrypter; import com.google.bitcoin.crypto.KeyCrypterException; import com.google.bitcoin.crypto.KeyCrypterScrypt; @@ -33,10 +31,11 @@ import com.google.bitcoin.utils.TestWithWallet; import com.google.bitcoin.utils.Threading; import com.google.bitcoin.wallet.KeyTimeCoinSelector; import com.google.bitcoin.wallet.WalletFiles; +import com.google.bitcoin.wallet.WalletTransaction; +import com.google.bitcoin.wallet.WalletTransaction.Pool; import com.google.common.collect.Lists; import com.google.common.util.concurrent.ListenableFuture; import com.google.protobuf.ByteString; - import org.bitcoinj.wallet.Protos; import org.bitcoinj.wallet.Protos.ScryptParameters; import org.bitcoinj.wallet.Protos.Wallet.EncryptionType; @@ -57,8 +56,8 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import static com.google.bitcoin.utils.TestUtils.*; import static com.google.bitcoin.core.Utils.*; +import static com.google.bitcoin.utils.TestUtils.*; import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.*; @@ -693,24 +692,47 @@ public class WalletTest extends TestWithWallet { } @Test - public void doubleSpendIdenticalTx() throws Exception { + public void doubleSpends() throws Exception { // Test the case where two semantically identical but bitwise different transactions double spend each other. + // We call the second transaction a "mutant" of the first. + // // This can (and has!) happened when a wallet is cloned between devices, and both devices decide to make the - // same spend simultaneously - for example due a re-keying operation. + // same spend simultaneously - for example due a re-keying operation. It can also happen if there are malicious + // nodes in the P2P network that are mutating transactions on the fly as occurred during Feb 2014. final BigInteger value = Utils.toNanoCoins(1, 0); - // Give us two outputs. - sendMoneyToWallet(value, AbstractBlockChain.NewBlockType.BEST_CHAIN); - sendMoneyToWallet(value, AbstractBlockChain.NewBlockType.BEST_CHAIN); final BigInteger value2 = Utils.toNanoCoins(2, 0); + // Give us three coins and make sure we have some change. + sendMoneyToWallet(value.add(value2), AbstractBlockChain.NewBlockType.BEST_CHAIN); // The two transactions will have different hashes due to the lack of deterministic signing, but will be // otherwise identical. Once deterministic signatures are implemented, this test will have to be tweaked. - Transaction send1 = checkNotNull(wallet.createSend(new ECKey().toAddress(params), value2)); - Transaction send2 = checkNotNull(wallet.createSend(new ECKey().toAddress(params), value2)); + final Address address = new ECKey().toAddress(params); + Transaction send1 = checkNotNull(wallet.createSend(address, value2)); + Transaction send2 = checkNotNull(wallet.createSend(address, value2)); send1 = roundTripTransaction(params, send1); wallet.commitTx(send2); + wallet.allowSpendingUnconfirmedTransactions(); + assertEquals(value, wallet.getBalance(Wallet.BalanceType.ESTIMATED)); + // Now spend the change. This transaction should die permanently when the mutant appears in the chain. + Transaction send3 = checkNotNull(wallet.createSend(address, value)); + wallet.commitTx(send3); assertEquals(BigInteger.ZERO, wallet.getBalance()); + final LinkedList dead = new LinkedList(); + final TransactionConfidence.Listener listener = new TransactionConfidence.Listener() { + @Override + public void onConfidenceChanged(Transaction tx, ChangeReason reason) { + final TransactionConfidence.ConfidenceType type = tx.getConfidence().getConfidenceType(); + if (reason == ChangeReason.TYPE && type == TransactionConfidence.ConfidenceType.DEAD) + dead.add(tx); + } + }; + send2.getConfidence().addEventListener(listener, Threading.SAME_THREAD); + send3.getConfidence().addEventListener(listener, Threading.SAME_THREAD); + // Double spend! sendMoneyToWallet(send1, AbstractBlockChain.NewBlockType.BEST_CHAIN); - assertEquals(BigInteger.ZERO, wallet.getBalance()); + // Back to having one coin. + assertEquals(value, wallet.getBalance()); + assertEquals(send2, dead.poll()); + assertEquals(send3, dead.poll()); } @Test