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 1df2ad79..cc2c7c94 100644 --- a/core/src/main/java/com/google/bitcoin/core/Wallet.java +++ b/core/src/main/java/com/google/bitcoin/core/Wallet.java @@ -728,6 +728,7 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi outpoints.add(input.getOutpoint()); } // Now for each pending transaction, see if it shares any outpoints with this tx. + LinkedList doubleSpentTxns = Lists.newLinkedList(); for (Transaction p : pending.values()) { for (TransactionInput input : p.getInputs()) { // This relies on the fact that TransactionOutPoint equality is defined at the protocol not object @@ -735,19 +736,15 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi TransactionOutPoint outpoint = input.getOutpoint(); if (outpoints.contains(outpoint)) { // It does, it's a double spend against the pending pool, which makes it relevant. - if (takeAction) { - // Look for the actual input object in tx that is double spending. - TransactionInput overridingInput = null; - for (TransactionInput txInput : tx.getInputs()) { - if (txInput.getOutpoint().equals(outpoint)) overridingInput = txInput; - } - killTx(tx, checkNotNull(overridingInput), p); - } - return true; + if (!doubleSpentTxns.isEmpty() && doubleSpentTxns.getLast() == p) continue; + doubleSpentTxns.add(p); } } } - return false; + if (takeAction) { + killTx(tx, doubleSpentTxns); + } + return !doubleSpentTxns.isEmpty(); } /** @@ -1082,50 +1079,47 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi } } - // Updates the wallet when a double spend occurs. - private void killTx(Transaction overridingTx, TransactionInput overridingInput, Transaction killedTx) { - final Sha256Hash killedTxHash = killedTx.getHash(); - if (overridingTx == null) { - // killedTx depended on a transaction that died because it was double spent or a coinbase that got re-orgd. - killedTx.getConfidence().setOverridingTransaction(null); - confidenceChanged.put(killedTx, TransactionConfidence.Listener.ChangeReason.TYPE); - pending.remove(killedTxHash); - unspent.remove(killedTxHash); - spent.remove(killedTxHash); - addWalletTransaction(Pool.DEAD, killedTx); - // TODO: Properly handle the recursive nature of killing transactions here. - return; + 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()); + log.warn("Disconnecting each input and moving connected transactions."); + pending.remove(tx.getHash()); + addWalletTransaction(Pool.DEAD, tx); + for (TransactionInput deadInput : tx.getInputs()) { + Transaction connected = deadInput.getOutpoint().fromTx; + if (connected == null) continue; + deadInput.disconnect(); + maybeMovePool(connected, "kill"); + } + tx.getConfidence().setOverridingTransaction(overridingTx); + confidenceChanged.put(tx, TransactionConfidence.Listener.ChangeReason.TYPE); } - TransactionOutPoint overriddenOutPoint = overridingInput.getOutpoint(); - // It is expected that we may not have the overridden/double-spent tx in our wallet ... in the (common?!) case - // where somebody is stealing money from us, the overriden tx belongs to someone else. - log.warn("Saw double spend of {} from chain override pending tx {}", - overriddenOutPoint, killedTx.getHashAsString()); - log.warn(" <-pending ->dead killed by {}", overridingTx.getHashAsString()); - pending.remove(killedTxHash); - addWalletTransaction(Pool.DEAD, killedTx); - log.info("Disconnecting inputs of the newly dead tx"); - for (TransactionInput deadInput : killedTx.getInputs()) { - Transaction connected = deadInput.getOutpoint().fromTx; - if (connected == null) continue; - deadInput.disconnect(); - maybeMovePool(connected, "kill"); - } - // Try and connect the overriding input to something in our wallet. It's expected that this will mostly fail - // because when somebody else is double-spending away a payment they made to us, we won't have the overridden - // tx as it's not ours to begin with. It'll only be found if we're double spending our own payments. - log.info("Trying to connect overriding tx back"); - TransactionInput.ConnectionResult result = overridingInput.connect(unspent, TransactionInput.ConnectMode.DISCONNECT_ON_CONFLICT); - if (result == TransactionInput.ConnectionResult.SUCCESS) { - maybeMovePool(overridingInput.getOutpoint().fromTx, "kill"); - } else { - result = overridingInput.connect(spent, TransactionInput.ConnectMode.DISCONNECT_ON_CONFLICT); + 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); if (result == TransactionInput.ConnectionResult.SUCCESS) { - maybeMovePool(overridingInput.getOutpoint().fromTx, "kill"); + maybeMovePool(input.getOutpoint().fromTx, "kill"); + } else { + result = input.connect(spent, TransactionInput.ConnectMode.DISCONNECT_ON_CONFLICT); + if (result == TransactionInput.ConnectionResult.SUCCESS) { + maybeMovePool(input.getOutpoint().fromTx, "kill"); + } } } - killedTx.getConfidence().setOverridingTransaction(overridingTx); - confidenceChanged.put(killedTx, TransactionConfidence.Listener.ChangeReason.TYPE); // TODO: Recursively kill other transactions that were double spent. } @@ -2308,7 +2302,6 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi Transaction tx = pair.tx; final Sha256Hash txHash = tx.getHash(); if (tx.isCoinBase()) { - log.warn("Coinbase tx {} -> dead", tx.getHash()); // All the transactions that we have in our wallet which spent this coinbase are now invalid // and will never confirm. Hopefully this should never happen - that's the point of the maturity // rule that forbids spending of coinbase transactions for 100 blocks. @@ -2319,7 +2312,7 @@ public class Wallet implements Serializable, BlockChainListener, PeerFilterProvi // can do our best. // // TODO: Is it better to try and sometimes fail, or not try at all? - killTx(null, null, tx); + killCoinbase(tx); } else { for (TransactionOutput output : tx.getOutputs()) { TransactionInput input = output.getSpentBy(); 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 2d57cd5f..c2f2724e 100644 --- a/core/src/test/java/com/google/bitcoin/core/WalletTest.java +++ b/core/src/test/java/com/google/bitcoin/core/WalletTest.java @@ -54,6 +54,7 @@ import java.util.concurrent.atomic.AtomicInteger; import static com.google.bitcoin.utils.TestUtils.*; import static com.google.bitcoin.core.Utils.*; +import static com.google.common.base.Preconditions.checkNotNull; import static org.junit.Assert.*; public class WalletTest extends TestWithWallet { @@ -562,6 +563,27 @@ public class WalletTest extends TestWithWallet { assertTrue(wallet.isConsistent()); } + @Test + public void doubleSpendIdenticalTx() throws Exception { + // Test the case where two semantically identical but bitwise different transactions double spend each other. + // 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. + 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); + // 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)); + send1 = roundTripTransaction(params, send1); + wallet.commitTx(send2); + assertEquals(BigInteger.ZERO, wallet.getBalance()); + sendMoneyToWallet(send1, AbstractBlockChain.NewBlockType.BEST_CHAIN); + assertEquals(BigInteger.ZERO, wallet.getBalance()); + } + @Test public void doubleSpendFinneyAttack() throws Exception { // A Finney attack is where a miner includes a transaction spending coins to themselves but does not