From 78dedcc9ba42a434eebc3cf9d82f461d8d121cda Mon Sep 17 00:00:00 2001 From: Mike Hearn Date: Thu, 27 Sep 2012 18:43:31 +0200 Subject: [PATCH] Re-organize how transaction confidence listeners end up being called. Ensure WalletEventListener.onTransactionConfidenceChanged is always called for every building transaction after every block. Resolves issue 251. --- .../com/google/bitcoin/core/Transaction.java | 16 +++-- .../bitcoin/core/TransactionConfidence.java | 9 ++- .../java/com/google/bitcoin/core/Wallet.java | 63 ++++++++++++------- .../com/google/bitcoin/core/WalletTest.java | 30 +++++++-- 4 files changed, 81 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/com/google/bitcoin/core/Transaction.java b/core/src/main/java/com/google/bitcoin/core/Transaction.java index 230bd5ea..6a5613a5 100644 --- a/core/src/main/java/com/google/bitcoin/core/Transaction.java +++ b/core/src/main/java/com/google/bitcoin/core/Transaction.java @@ -242,14 +242,14 @@ public class Transaction extends ChildMessage implements Serializable { } /** - * Puts the given block in the internal serializable set of blocks in which this transaction appears. This is + *

Puts the given block in the internal serializable set of blocks in which this transaction appears. This is * used by the wallet to ensure transactions that appear on side chains are recorded properly even though the - * block stores do not save the transaction data at all.

+ * block stores do not save the transaction data at all.

* *

If there is a re-org this will be called once for each block that was previously seen, to update which block - * is the best chain. The best chain block is guaranteed to be called last. So this must be idempotent. + * is the best chain. The best chain block is guaranteed to be called last. So this must be idempotent.

* - *

Sets updatedAt to be the earliest valid block time where this tx was seen + *

Sets updatedAt to be the earliest valid block time where this tx was seen.

* * @param block The {@link StoredBlock} in which the transaction has appeared. * @param bestChain whether to set the updatedAt timestamp from the block header (only if not already set) @@ -269,10 +269,14 @@ public class Transaction extends ChildMessage implements Serializable { transactionConfidence.setAppearedAtChainHeight(block.getHeight()); // Reset the confidence block depth. - transactionConfidence.setDepthInBlocks(0); + transactionConfidence.setDepthInBlocks(1); // Reset the work done. - transactionConfidence.setWorkDone(BigInteger.ZERO); + try { + transactionConfidence.setWorkDone(block.getHeader().getWork()); + } catch (VerificationException e) { + throw new RuntimeException(e); // Cannot happen. + } // The transaction is now on the best chain. transactionConfidence.setConfidenceType(ConfidenceType.BUILDING); 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 cb670673..ce52f6d4 100644 --- a/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java +++ b/core/src/main/java/com/google/bitcoin/core/TransactionConfidence.java @@ -49,7 +49,7 @@ import java.util.Set; *

Alternatively, you may know that the transaction is "dead", that is, one or more of its inputs have * been double spent and will never confirm unless there is another re-org.

* - *

TransactionConfidence is updated via the {@link com.google.bitcoin.core.TransactionConfidence#notifyWorkDone()} + *

TransactionConfidence is updated via the {@link com.google.bitcoin.core.TransactionConfidence#notifyWorkDone(Block)} * method to ensure the block depth and work done are up to date.

* To make a copy that won't be changed, use {@link com.google.bitcoin.core.TransactionConfidence#duplicate()}. */ @@ -93,8 +93,10 @@ public class TransactionConfidence implements Serializable { public synchronized void addEventListener(Listener listener) { Preconditions.checkNotNull(listener); if (listeners == null) - listeners = new ArrayList(1); - listeners.add(listener); + listeners = new ArrayList(2); + // Dedupe registrations. This makes the wallet code simpler. + if (!listeners.contains(listener)) + listeners.add(listener); } public synchronized void removeEventListener(Listener listener) { @@ -298,6 +300,7 @@ public class TransactionConfidence implements Serializable { if (getConfidenceType() == ConfidenceType.BUILDING) { this.depth++; this.workDone = this.workDone.add(block.getWork()); + runListeners(); } } 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 f11367cd..68a85968 100644 --- a/core/src/main/java/com/google/bitcoin/core/Wallet.java +++ b/core/src/main/java/com/google/bitcoin/core/Wallet.java @@ -176,6 +176,10 @@ public class Wallet implements Serializable { // A listener that relays confidence changes from the transaction confidence object to the wallet event listener, // as a convenience to API users so they don't have to register on every transaction themselves. private transient TransactionConfidence.Listener txConfidenceListener; + // If a TX hash appears in this set then notifyNewBestBlock will ignore it, as its confidence was already set up + // in receive() via Transaction.setBlockAppearance(). As the BlockChain always calls notifyNewBestBlock even if + // it sent transactions to the wallet, without this we'd double count. + private transient HashSet ignoreNextNewBlock; /** * Creates a new, empty wallet with no keys and no transactions. If you want to restore a wallet from disk instead, @@ -194,6 +198,7 @@ public class Wallet implements Serializable { private void createTransientState() { eventListeners = new ArrayList(); + ignoreNextNewBlock = new HashSet(); txConfidenceListener = new TransactionConfidence.Listener() { public void onConfidenceChanged(Transaction tx) { invokeOnTransactionConfidenceChanged(tx); @@ -723,14 +728,12 @@ public class Wallet implements Serializable { if (valueSentToMe.equals(BigInteger.ZERO)) { // There were no change transactions so this tx is fully spent. log.info(" ->spent"); - boolean alreadyPresent = spent.put(tx.getHash(), tx) != null; - checkState(!alreadyPresent, "TX in both pending and spent pools"); + addWalletTransaction(Pool.SPENT, tx); } else { // There was change back to us, or this tx was purely a spend back to ourselves (perhaps for // anonymization purposes). log.info(" ->unspent"); - boolean alreadyPresent = unspent.put(tx.getHash(), tx) != null; - checkState(!alreadyPresent, "TX in both pending and unspent pools"); + addWalletTransaction(Pool.UNSPENT, tx); } } else if (sideChain) { // The transaction was accepted on an inactive side chain, but not yet by the best chain. @@ -756,9 +759,12 @@ public class Wallet implements Serializable { // we don't need to consider this transaction inactive, we can just ignore it. } else { log.info(" ->inactive"); - inactive.put(tx.getHash(), tx); + addWalletTransaction(Pool.INACTIVE, tx); } } else if (bestChain) { + // Saw a non-pending transaction appear on the best chain, ie, we are replaying the chain or a spend + // that we never saw broadcast (and did not originate) got included. + // // This can trigger tx confidence listeners to be run in the case of double spends. We may need to // delay the execution of the listeners until the bottom to avoid the wallet mutating during updates. processTxFromBestChain(tx); @@ -771,9 +777,16 @@ public class Wallet implements Serializable { // in turn allowed to re-enter the Wallet. This means we cannot assume anything about the state of the wallet // from now on. The balance just received may already be spent. - // Mark the tx as appearing in this block so we can find it later after a re-org. if (block != null) { + // Mark the tx as appearing in this block so we can find it later after a re-org. This also tells the tx + // confidence object about the block and sets its work done/depth appropriately. tx.setBlockAppearance(block, bestChain); + if (bestChain) { + // Don't notify this tx of work done in notifyNewBestBlock which will be called immediately after + // this method has been called by BlockChain for all relevant transactions. Otherwise we'd double + // count. + ignoreNextNewBlock.add(txHash); + } } // Inform anyone interested that we have received or sent coins but only if: @@ -823,10 +836,16 @@ public class Wallet implements Serializable { // This is so that they can update their work done and depth. Set transactions = getTransactions(true, false); for (Transaction tx : transactions) { - tx.getConfidence().notifyWorkDone(block); + if (ignoreNextNewBlock.contains(tx.getHash())) { + // tx was already processed in receive() due to it appearing in this block, so we don't want to + // notify the tx confidence of work done twice, it'd result in miscounting. + ignoreNextNewBlock.remove(tx.getHash()); + } else { + tx.getConfidence().notifyWorkDone(block); + } } + queueAutoSave(); } - queueAutoSave(); } /** @@ -859,13 +878,11 @@ public class Wallet implements Serializable { if (!tx.getValueSentToMe(this).equals(BigInteger.ZERO)) { // It's sending us coins. log.info(" new tx {} ->unspent", tx.getHashAsString()); - boolean alreadyPresent = unspent.put(tx.getHash(), tx) != null; - checkState(!alreadyPresent, "TX was received twice"); + addWalletTransaction(Pool.UNSPENT, tx); } else if (!tx.getValueSentFromMe(this).equals(BigInteger.ZERO)) { // It spent some of our coins and did not send us any. log.info(" new tx {} ->spent", tx.getHashAsString()); - boolean alreadyPresent = spent.put(tx.getHash(), tx) != null; - checkState(!alreadyPresent, "TX was received twice"); + addWalletTransaction(Pool.SPENT, tx); } else { // It didn't send us coins nor spend any of our coins. If we're processing it, that must be because it // spends outpoints that are also spent by some pending transactions - maybe a double spend of somebody @@ -880,7 +897,7 @@ public class Wallet implements Serializable { log.warn("Saw double spend from chain override pending tx {}", doubleSpend.getHashAsString()); log.warn(" <-pending ->dead"); pending.remove(doubleSpend.getHash()); - dead.put(doubleSpend.getHash(), doubleSpend); + addWalletTransaction(Pool.DEAD, doubleSpend); // Inform the event listeners of the newly dead tx. doubleSpend.getConfidence().setOverridingTransaction(tx); } @@ -1017,6 +1034,7 @@ public class Wallet implements Serializable { // spends. updateForSpends(tx, false); // Add to the pending pool. It'll be moved out once we receive this transaction on the best chain. + // This also registers txConfidenceListener so wallet listeners get informed. log.info("->pending: {}", tx.getHashAsString()); addWalletTransaction(Pool.PENDING, tx); @@ -1097,33 +1115,34 @@ public class Wallet implements Serializable { } /** - * Adds the given transaction to the given pools and registers a confidence change listener on it. Not to be used - * when moving txns between pools. + * Adds the given transaction to the given pools and registers a confidence change listener on it. */ private synchronized void addWalletTransaction(Pool pool, Transaction tx) { switch (pool) { case UNSPENT: - unspent.put(tx.getHash(), tx); + Preconditions.checkState(unspent.put(tx.getHash(), tx) == null); break; case SPENT: - spent.put(tx.getHash(), tx); + Preconditions.checkState(spent.put(tx.getHash(), tx) == null); break; case PENDING: - pending.put(tx.getHash(), tx); + Preconditions.checkState(pending.put(tx.getHash(), tx) == null); break; case DEAD: - dead.put(tx.getHash(), tx); + Preconditions.checkState(dead.put(tx.getHash(), tx) == null); break; case INACTIVE: - inactive.put(tx.getHash(), tx); + Preconditions.checkState(inactive.put(tx.getHash(), tx) == null); break; case PENDING_INACTIVE: - pending.put(tx.getHash(), tx); - inactive.put(tx.getHash(), tx); + Preconditions.checkState(pending.put(tx.getHash(), tx) == null); + Preconditions.checkState(inactive.put(tx.getHash(), tx) == null); break; default: throw new RuntimeException("Unknown wallet transaction type " + pool); } + // This is safe even if the listener has been added before, as TransactionConfidence ignores duplicate + // registration requests. That makes the code in the wallet simpler. tx.getConfidence().addEventListener(txConfidenceListener); } 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 7c5de0a0..594c19d9 100644 --- a/core/src/test/java/com/google/bitcoin/core/WalletTest.java +++ b/core/src/test/java/com/google/bitcoin/core/WalletTest.java @@ -27,6 +27,7 @@ import org.junit.Test; import java.io.File; import java.math.BigInteger; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -211,6 +212,7 @@ public class WalletTest { // Test that we correctly process transactions arriving from the chain, with callbacks for inbound and outbound. final BigInteger bigints[] = new BigInteger[4]; final Transaction txn[] = new Transaction[2]; + final LinkedList confTxns = new LinkedList(); wallet.addEventListener(new AbstractWalletEventListener() { @Override public void onCoinsReceived(Wallet wallet, Transaction tx, BigInteger prevBalance, BigInteger newBalance) { @@ -227,14 +229,22 @@ public class WalletTest { bigints[3] = newBalance; txn[1] = tx; } + + @Override + public void onTransactionConfidenceChanged(Wallet wallet, Transaction tx) { + super.onTransactionConfidenceChanged(wallet, tx); + confTxns.add(tx); + } }); // Receive some money. BigInteger oneCoin = Utils.toNanoCoins(1, 0); Transaction tx1 = createFakeTx(params, oneCoin, myAddress); - StoredBlock b1 = createFakeBlock(params, blockStore, tx1).storedBlock; - wallet.receiveFromBlock(tx1, b1, BlockChain.NewBlockType.BEST_CHAIN); + BlockPair b1 = createFakeBlock(params, blockStore, tx1); + wallet.receiveFromBlock(tx1, b1.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(b1.block); assertEquals(null, txn[1]); // onCoinsSent not called. + assertEquals(tx1, confTxns.getFirst()); // onTransactionConfidenceChanged called assertEquals(txn[0].getHash(), tx1.getHash()); assertEquals(BigInteger.ZERO, bigints[0]); assertEquals(oneCoin, bigints[1]); @@ -246,10 +256,13 @@ public class WalletTest { // want to get back to our previous state. We can do this by just not confirming the transaction as // createSend is stateless. txn[0] = txn[1] = null; - StoredBlock b2 = createFakeBlock(params, blockStore, send1).storedBlock; - wallet.receiveFromBlock(send1, b2, BlockChain.NewBlockType.BEST_CHAIN); + confTxns.clear(); + BlockPair b2 = createFakeBlock(params, blockStore, send1); + wallet.receiveFromBlock(send1, b2.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(b2.block); assertEquals(bitcoinValueToFriendlyString(wallet.getBalance()), "0.90"); assertEquals(null, txn[0]); + assertEquals(2, confTxns.size()); assertEquals(txn[1].getHash(), send1.getHash()); assertEquals(bitcoinValueToFriendlyString(bigints[2]), "1.00"); assertEquals(bitcoinValueToFriendlyString(bigints[3]), "0.90"); @@ -257,9 +270,14 @@ public class WalletTest { Transaction send2 = wallet.createSend(new ECKey().toAddress(params), toNanoCoins(0, 10)); // What we'd really like to do is prove the official client would accept it .... no such luck unfortunately. wallet.commitTx(send2); - StoredBlock b3 = createFakeBlock(params, blockStore, send2).storedBlock; - wallet.receiveFromBlock(send2, b3, BlockChain.NewBlockType.BEST_CHAIN); + BlockPair b3 = createFakeBlock(params, blockStore, send2); + wallet.receiveFromBlock(send2, b3.storedBlock, BlockChain.NewBlockType.BEST_CHAIN); + wallet.notifyNewBestBlock(b3.block); assertEquals(bitcoinValueToFriendlyString(wallet.getBalance()), "0.80"); + Block b4 = createFakeBlock(params, blockStore).block; + confTxns.clear(); + wallet.notifyNewBestBlock(b4); + assertEquals(3, confTxns.size()); } @Test